Repository navigation
fix: Use Websocket ping - pong to detect a stuck connection - #7168
Conversation
53d19a7 to
8369166
Compare
| // going through the socket | ||
| static constexpr std::chrono::seconds HEALTH_CHECK_INTERVAL_LONG = | ||
| std::chrono::seconds{60}; | ||
| // Use a short intervat when there is no traffic except for the |
There was a problem hiding this comment.
| // Use a short intervat when there is no traffic except for the | |
| // Use a short interval when there is no traffic except for the |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
|
||
| this->stream.control_callback( | ||
| boost::bind(&WebSocketConnectionHelper::onControlFrame, this, | ||
| boost::placeholders::_1, boost::placeholders::_2)); |
There was a problem hiding this comment.
warning: prefer a lambda to boost::bind [modernize-avoid-bind]
| boost::placeholders::_1, boost::placeholders::_2)); | |
| [this] { onControlFrame(boost::placeholders::_1, boost::placeholders::_2); }); |
| this->healthCheckTimer.expires_after(HEALTH_CHECK_INTERVAL_LONG); | ||
| this->healthCheckTimer.async_wait( | ||
| boost::bind(&WebSocketConnectionHelper::onHealthCheck, | ||
| this->shared_from_this(), boost::placeholders::_1)); |
There was a problem hiding this comment.
warning: prefer a lambda to boost::bind [modernize-avoid-bind]
| this->shared_from_this(), boost::placeholders::_1)); | |
| [capture0 = this->shared_from_this()] { capture0->onHealthCheck(boost::placeholders::_1); }); |
| return; | ||
| } | ||
|
|
||
| this->stream.async_ping({}, [](const boost::system::error_code ec) { |
There was a problem hiding this comment.
warning: Missing reference on large type (sizeof const boost::system::error_code ec is 24 bytes) [clazy-function-args-by-ref]
| this->stream.async_ping({}, [](const boost::system::error_code ec) { | |
| this->stream.async_ping({}, [](const boost::system::error_code &ec) { |
| this->healthCheckTimer.expires_after(HEALTH_CHECK_INTERVAL_SHORT); | ||
| this->healthCheckTimer.async_wait( | ||
| boost::bind(&WebSocketConnectionHelper::onHealthCheck, | ||
| this->shared_from_this(), boost::placeholders::_1)); |
There was a problem hiding this comment.
warning: prefer a lambda to boost::bind [modernize-avoid-bind]
| this->shared_from_this(), boost::placeholders::_1)); | |
| [capture0 = this->shared_from_this()] { capture0->onHealthCheck(boost::placeholders::_1); }); |
| BalancedResolverResults resolvedEndpoints; | ||
|
|
||
| boost::asio::steady_timer healthCheckTimer; | ||
| int pingProbesTried; |
There was a problem hiding this comment.
Use an unsigned type and initialize it here.
| // Use a short interval when there is no traffic except for the | ||
| // ping probe so that we can detect a failed socket quickly | ||
| static constexpr std::chrono::seconds HEALTH_CHECK_INTERVAL_SHORT = | ||
| std::chrono::seconds{5}; |
There was a problem hiding this comment.
The comment and the variable name are confusing. They make it sounds like the ping check has two different modes that depend on the application messages. So if the application doesn't send/receive anything, we'd send pings every 5s.
Looking at the code, that's not what we do.
There was a problem hiding this comment.
True, the current text is a bit misleading. Updated phrasing should describe the actual logic correctly.
| boost::bind(&WebSocketConnectionHelper::onControlFrame, this, | ||
| boost::placeholders::_1, boost::placeholders::_2)); |
There was a problem hiding this comment.
Why can't this and the one below use beast::bind_front_handler?
There was a problem hiding this comment.
I just replaced the binds with lambdas.
| if (frame_type == boost::beast::websocket::frame_type::pong) | ||
| { | ||
| this->pingProbesTried = 0; | ||
| this->healthCheckTimer.expires_after(HEALTH_CHECK_INTERVAL_LONG); |
There was a problem hiding this comment.
Whenever we call expires_after on a steady timer, it will cancel any existing async_wait calls. This means onHealthCheck will never be called after this.
We will need some mechanism to handle this case - either don't reset the timer, or explicitly call async_wait again
There was a problem hiding this comment.
Dang, such a lame mistake :) It should be okay now.
FTR it is possible to not see the periodic pinging if there is other data being regularly received over the socket. But that doesn't seem to happen in practice.
connection health with a ping-pong
- Init ping probe count in the class header
113b6aa to
608b2d2
Compare
If a Websocket connection fails in a way that the connection appears serviceable from the application's perspective but cannot really transfer any data, there is currently no way for us to detect this. This may happen after some network configuration changes or if there is an issue somewhere along the way between us and the remote peer.
We can leverage Websocket ping and pong frames to periodically probe the remote peer if we do not see any other traffic going through the Websocket connection.