Bring back client expecting heartbeats #231
Conversation
| private activeHeartbeatHandle?: ReturnType<typeof setInterval> | undefined; | ||
| private activeHeartbeatMisses = 0; | ||
|
|
||
| private passiveHearbeatHandle?: ReturnType<typeof setTimeout> | undefined; |
There was a problem hiding this comment.
I'm not a fan of the same class here being used in different ways by the client and the server, ideally we'd have a ClientSessionConnected and ServerSessionConnected but that seems like a can of warms.
There was a problem hiding this comment.
big same, i asked to split this state due to the presence / absence of the heartbeat handle being significant state deviation.
|
working on a test, but wanted to get some 👀 |
| private activeHeartbeatHandle?: ReturnType<typeof setInterval> | undefined; | ||
| private activeHeartbeatMisses = 0; | ||
|
|
||
| private passiveHearbeatHandle?: ReturnType<typeof setTimeout> | undefined; |
There was a problem hiding this comment.
big same, i asked to split this state due to the presence / absence of the heartbeat handle being significant state deviation.
I like that it's contained within the state machine so that we don't have to deal with state that only belongs to connected sessions outside of connected sessions. Like it'd be unfortunate if we forgot the heartbeat timer while we're transparently reconnecting and then killed the connection or whatever. Things are self contained now, it's great |
oh i mean the timer would still be in the state machine just the handling of the timer events is outside of it |
| private activeHeartbeatHandle?: ReturnType<typeof setInterval> | undefined; | ||
| private activeHeartbeatMisses = 0; | ||
|
|
||
| private passiveHearbeatHandle?: ReturnType<typeof setTimeout> | undefined; |
There was a problem hiding this comment.
| private passiveHearbeatHandle?: ReturnType<typeof setTimeout> | undefined; | |
| private passiveHeartbeatHandle?: ReturnType<typeof setTimeout> | undefined; |
There was a problem hiding this comment.
Not a fan of pirate speak? 😆
Good catch
Why
We removed the client being required to send heartbeat on interval due to browser timer throttling in #220. That makes sense, since we don't want the server to expect anything from the client that relies on client timers, making the timer server authoritative makes sense.
That said, this entirely removed our ability to detect bad servers or phantom disconnects on the client.
What changed
Add a client timer to expect a heartbeat from the server every
options.heartbeatsUntilDead * options.heartbeatIntervalMs, if we don't see a heartbeat, we close the connection.This is fine in the context of browser throttling timers since in the worst case of throttling we're holding on a connection for too long, and in the best case we can detect bad connections.
Versioning
Internal change.