More responsive to network outages #360
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
2 Skipped Deployments
|
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
👍 Looks good to me! Reviewed everything up to f9e68ca in 45 seconds
More details
- Looked at
272lines of code in2files - Skipped
0files when reviewing. - Skipped posting
3drafted comments based on config settings.
1. js-pkg/client/src/sleeper.ts:6
- Draft comment:
Therejectfunction is defined but never used. Consider removing it to avoid confusion. - Reason this comment was not posted:
Confidence changes required:50%
The reject function is defined but never used. This is unnecessary and could be misleading.
2. js-pkg/client/src/provider.ts:333
- Draft comment:
EnsurereconnectSleeperis not already in use before reassigning it to avoid potential issues. - Reason this comment was not posted:
Comment did not seem useful.
3. js-pkg/client/src/provider.ts:343
- Draft comment:
EnsurereconnectSleeperis not already in use before reassigning it to avoid potential issues. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_13iEYblxATMs42Ye
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
To sum up the comments, I think we can simplify this a little bit by:
- Rather than being able to "set" and "clear" a heartbeat, just have one operation — "reset", which clears any heartbeat timeout and sets another one. That gets called on websocket connection and on receiving a message. (We might still want a "clear" that gets called in situations where we don't want to set another one, like actually going offline).
- Rather than just looking for a connection timeout on specific sync messages, set it on every sent message. This means we'll initiate the reconnection flow if it takes too long to get a response to any message. (This isn't technically true: if we send two messages and then get one response, it will clear the connection timeout without setting another, but we'd fall back to the heartbeat anyway in that scenario).
Also, I can't comment on lines that haven't changed, but I think we should clear both the heartbeat and connection timeout in websocketClose and websocketError
js-pkg/client/src/provider.ts
Outdated
| let hasLocalChanges = this.ackedVersion !== this.localVersion | ||
| if (hasLocalChanges === this.hasLocalChanges) { | ||
| return | ||
| } |
There was a problem hiding this comment.
This just occurred to me now — could we remove a bit of state by making a getter to derive hasLocalChanges? Like, within the class:
get hasLocalChanges() {
return this.ackedVersion !== this.localVersion
}Thinking it over now, I'm not actually 100% clear when local-changes should get emitted — it seems like we go through this code path when both sending and receiving a sync message?
There was a problem hiding this comment.
I like that, done. Unfortunately it adds a bit of complexity at the former call sites because we want to emit if and only if the hasLocalChanges value would change, but I do think it's an improvement overall.
js-pkg/client/src/provider.ts
Outdated
| // doesn't mean we entirely true the browser, since it can be wrong | ||
| // (e.g. in the case that the connection is over localhost). | ||
| this.checkSync() | ||
| this.setConnectionTimeout() |
There was a problem hiding this comment.
Do you think there's a drawback to just calling this inside send rather than here and in the setHeartbeat callback? It would get called in both paths anyway via checkSync, plus that way we'd initiate the reconnection flow if the server takes too long to respond to any message (rather than just specific sync messages). We could rename MAX_TIMEOUT_WITHOUT_RECEIVING_HEARTBEAT to MAX_RESPONSE_TIMEOUT or something.
There was a problem hiding this comment.
The trouble with that is that not every message necessarily has a response, so we might encounter false positives.
That said, I have moved the call into checkSync, since we know that every checkSync call should return a response. This also should speed up offline detection if a user is interacting with the state when the connection is dropped.
js-pkg/client/src/provider.ts
Outdated
| private clearHeartbeat() { | ||
| if (this.heartbeatHandle) { | ||
| clearTimeout(this.heartbeatHandle) | ||
| this.heartbeatHandle = null | ||
| } | ||
| } | ||
|
|
||
| private setHeartbeat() { | ||
| if (this.heartbeatHandle) { | ||
| return | ||
| } | ||
| this.heartbeatHandle = setTimeout(() => { | ||
| this.checkSync() | ||
| this.setConnectionTimeout() | ||
| this.heartbeatHandle = null | ||
| }, MAX_TIMEOUT_BETWEEN_HEARTBEATS) | ||
| } |
There was a problem hiding this comment.
Trying to think if there's ever a scenario in which we want to bail out of setting a heartbeat if there's one queued, rather than just resetting it? I think we basically always want a heartbeat going and to just keep "moving the timer up" when we receive messages from the server.
I'd change these methods like this:
| private clearHeartbeat() { | |
| if (this.heartbeatHandle) { | |
| clearTimeout(this.heartbeatHandle) | |
| this.heartbeatHandle = null | |
| } | |
| } | |
| private setHeartbeat() { | |
| if (this.heartbeatHandle) { | |
| return | |
| } | |
| this.heartbeatHandle = setTimeout(() => { | |
| this.checkSync() | |
| this.setConnectionTimeout() | |
| this.heartbeatHandle = null | |
| }, MAX_TIMEOUT_BETWEEN_HEARTBEATS) | |
| } | |
| private clearHeartbeat() { | |
| if (this.heartbeatHandle) { | |
| clearTimeout(this.heartbeatHandle) | |
| this.heartbeatHandle = null | |
| } | |
| } | |
| private resetHeartbeat() { | |
| this.clearHeartbeat() | |
| this.heartbeatHandle = setTimeout(() => { | |
| this.checkSync() | |
| this.setConnectionTimeout() | |
| this.heartbeatHandle = null | |
| }, MAX_TIMEOUT_BETWEEN_HEARTBEATS) | |
| } |
And then we should probably call clearHeartbeat in websocketClose and websocketError.
Co-authored-by: Jake Lazaroff <[email protected]>
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 2ece974 in 20 seconds
More details
- Looked at
28lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. js-pkg/client/src/provider.ts:272
- Draft comment:
setConnectionTimeoutis called multiple times unnecessarily, which can lead to redundant operations. Consider refactoring to ensure it's only called when needed. - Reason this comment was not posted:
Comment looked like it was already resolved.
Workflow ID: wflow_PHWYw7aXZvzIJ0js
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on eff7cd2 in 27 seconds
More details
- Looked at
92lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. js-pkg/client/src/provider.ts:228
- Draft comment:
The comment here is misleading. It suggests that the local version is incremented before emitting, but the emission happens after the increment. Consider revising the comment to accurately reflect the method's behavior. - Reason this comment was not posted:
Comment looked like it was already resolved.
2. js-pkg/client/src/provider.ts:242
- Draft comment:
The comment here is misleading. It suggests that the local version is incremented before emitting, but the method does not increment the local version. Consider revising the comment to accurately reflect the method's behavior. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_okh5oQZ2BCRje4Qz
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 036b06e in 3 seconds
More details
- Looked at
13lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. js-pkg/client/src/provider.ts:244
- Draft comment:
The change from(version === this.localVersion)toversion === this.localVersionis stylistic and doesn't affect functionality. Ensure this aligns with your coding standards. - Reason this comment was not posted:
Confidence changes required:10%
The PR description mentions a change in the way localVersion is handled, but the code change is minimal and doesn't seem to address the main points of the description.
Workflow ID: wflow_I37hJoJPfVxHeozt
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 81f66ff in 3 seconds
More details
- Looked at
36lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. js-pkg/client/src/provider.ts:190
- Draft comment:
TheresetHeartbeatmethod is a good addition for managing heartbeats. Ensure that this method is consistently used wherever a heartbeat reset is needed, such as inwebsocketOpenandreceiveMessage. This helps maintain a single point of control for heartbeat logic. - Reason this comment was not posted:
Confidence changes required:20%
The PR introduces a new methodresetHeartbeatwhich callsclearHeartbeatand then sets a new timeout. This is a good approach to ensure that the heartbeat is reset properly. However, theresetHeartbeatmethod is called in multiple places, and it's important to ensure that the logic is consistent and doesn't introduce any unintended side effects.
Workflow ID: wflow_5HUULJRmWSnFMp2A
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 649289c in 20 seconds
More details
- Looked at
20lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. js-pkg/client/src/provider.ts:489
- Draft comment:
Consider adding a check to ensure only one reconnection attempt is made at a time to prevent multiple concurrent reconnection attempts. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_RY5N2YkbzzTroLux
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
Thanks for the review! I think I did all of this, except for one thing: instead of setting the connection timeout only on every message type, I do it on every sync message. This way if we send some message that the server doesn't need to acknowledge, we don't accidentally disconnect. This change still cleans up the logic a bit. |
| encoding.writeVarUint8Array(encoder, encoding.toUint8Array(versionEncoder)) | ||
|
|
||
| this.send(encoding.toUint8Array(encoder)) | ||
|
|
||
| this.updateSyncedState() |
There was a problem hiding this comment.
Just making sure this is intentional — we do not want to emit the local-changes event here?
There was a problem hiding this comment.
that's right, this function is now responsible for checking if the current localVersion has been seen by the server. Since it is no longer responsible for incrementing localVersion, it can no longer lead to the local sync status changing -- it is the responsibility of the caller to increment localVersion and emit the event if necessary.
Co-authored-by: Jake Lazaroff <[email protected]>
Co-authored-by: Jake Lazaroff <[email protected]>
Co-authored-by: Jake Lazaroff <[email protected]>
paulgb
deleted the
paul/dis-3029-make-y-sweet-more-responsive-to-network-disconnections
branch
This PR introduces a number of improvements that make
YSweetProvidermore responsive to network conditions.The main issue this attempts to address is that browsers are surprisingly inconsistent in how long it takes to terminate a WebSocket connection when the network is lost. I have observed the timeout taking anywhere from 10s (Firefox) to 95s (Chrome) when using a VPN. (Without the VPN it is a bit better, presumably because the VPN makes the network status more opaque to the browser.)
Passive / active connection monitoring
This PR introduces passive connection monitoring that switches into an active heartbeat when message volume is low.
When the server is sending the client lots of messages (e.g. frequent awareness updates), we know the connection is alive, and don't have to send any sort of heartbeat to confirm it.
When we haven't received a message for a certain time threshold (
MAX_TIMEOUT_BETWEEN_HEARTBEATS), we don't know whether it's because the connection was lost or just a lull in events, so we start to send an active heartbeat. This uses the same mechanism on the server introduced for detecting local unsaved changes: we simply re-send the latestlocalVersion(formerlylastSyncSent) instead of incrementing it (since the underlying data hasn't changed).Browser connection status monitoring
Browsers also provide a mechanism for being notified about online/offline state. It's not entirely reliable, but we can use it as an additional signal to accelerate reconnection. The way it works is:
offlineevent, we immediately send a heartbeat, which means we only have to waitMAX_TIMEOUT_WITHOUT_RECEIVING_HEARTBEATinstead of up toMAX_TIMEOUT_BETWEEN_HEARTBEATS + MAX_TIMEOUT_WITHOUT_RECEIVING_HEARTBEATbefore showing as offline.onlineevent, if we are currently waiting to retry a network operation (either obtaining credentials or connecting to a websocket), we bypass the wait and go directly to making the request.