Improve websocket liveness checks and idle timeout handling#5517
Conversation
- Idle timeout no longer requires receive activity alone: the send loop extends the idle deadline whenever it makes write progress (rate-limited to once per second). - On idle timeout, initiate a proper close handshake carrying an "idle timeout" reason instead of abruptly tearing down the connection, so well-behaved clients can tell why they were disconnected. The connection is torn down if the handshake does not complete within a 10s grace period. - The "websocket connection aborted" warning and ws_clients_aborted metric now fire only when the client actor task is actually aborted or panics, not on every normal disconnect. - Fix the kick log message to report the channel's configured capacity instead of its remaining capacity (always 0 at that point).
aasoni
force-pushed
the
aasoni/ws-idle-timeout-rework
branch
from
e4fc66d to
234f6c9
Compare
| fut.await | ||
| fut.await; | ||
|
|
||
| scopeguard::ScopeGuard::into_inner(abort_guard); |
There was a problem hiding this comment.
I don't we need the changes to the scope guard. I updated the other PR so that we only log a warning if we hadn't previously recorded a more specific cause ( https://github.com/clockworklabs/SpacetimeDB/pull/5495/changes#diff-cde382f76a7190bac09b3ca01dc55e56abb79225fb34ec657d6e43ba7792da0eR892), and I'd rather maintain that the ws_clients_aborted metric always gets incremented if the ws_clients_spawned counter gets incremented. Once we deploy these changes and verify that the new stats are adding up to match ws_clients_aborted we could just remove it entirely.
| log::warn!("error sending frame: {e:#}"); | ||
| break 'outer; | ||
| } | ||
| // Writing succeeded, so the client is making progress: |
There was a problem hiding this comment.
If I'm reading this correctly, completing a ws.feed doesn't mean that the client is making progress. It just means that the buffer isn't full.
It seems like that could result in us leaving a connection open for a pretty long time, as long as the client is periodically being sent very small messages. It also means our threshold for being active depends on buffer sizes (including any proxies), which makes it easy to accidentally change the behavior.
I think it would be better for us to just increase the idle timeout rather than counting these writes as activity.
2 participants
Description of Changes
Idle timeout no longer requires a pong to restart, the send loop extends the idle deadline whenever it makes write progress (rate-limited to once per second). Note: it has been pointed out that if a client dies, write progress can still be made because the LB will continue to read from the socket until its own buffer fills up. This is considered ok as it will avoid aborting slow clients while keeping around idle clients for some extra time while the LB buffer fills up.
On idle timeout, initiate a proper close handshake carrying an "idle timeout" reason instead of abruptly tearing down the connection, so well-behaved clients can tell why they were disconnected. The connection is torn down if the handshake does not complete within a 10s grace period.
The "websocket connection aborted" warning and ws_clients_aborted metric now fire only when the client actor task is actually aborted or panics, not on every normal disconnect.
Fix the kick log message to report the channel's configured capacity instead of its remaining capacity (always 0 at that point).
API and ABI breaking changes
No
Expected complexity level and risk
3
Testing
These changes were made on the BitCraft cluster and were tested by hundreds of players connecting and subscribing to large amounts of data. I am fairly confident the change doesn't break things. I am less confident it's the disconnection silver bullet, but it may help.