fix(comlink): ensure nodes always accept handshake synchronize events #2163
+58
−29
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
azaxarov
reviewed
Nov 14, 2024
| @@ -260,8 +260,7 @@ export const createController = (input: {targetOrigin: string}): Controller => { | |||
|
|
|||
| const stop = () => { | |||
| channels.forEach((channel) => { | |||
| channel.disconnect() | |||
| channel.stop() | |||
| cleanupChannel(channel as unknown as Channel<Message, Message>) | |||
There was a problem hiding this comment.
Will the following also work?
Suggested change
| cleanupChannel(channel as unknown as Channel<Message, Message>) | |
| cleanupChannel(channel) |
There was a problem hiding this comment.
Unfortunately not as it stands, I guess this has to do with type variance? channel here is of type Channel<R, S>, and that isn't considered a subtype of Channel<Message, Message> even if R and S extend Message...or something like that.
I need to look at improving the typing in places, if you have any suggestions they'd be very welcome! 😊
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently, comlink's node state machine will only handle syn handshake events when in an
idlestate. In some situations, the controller or some of the channels it maintains may detect a loss of connection and transition to ahandshakingstate, the disconnect event emitted at this point may then fail to reach the relevant node(s), and thus the node will remain in aconnectedstate, as the nodes themselves only blindly respond to heartbeat events.This PR adds a change so that the node state machine will always transition to its internal
handshakingstate when receiving a syn handshake event, regardless of the state it is currently in. This should greatly improve the success rate of attempt reconnects. For example you may have noticed in our test environments that the connection is often lost after a HMR, this should no longer be the case.It also drops the default expected response timeout to 3s from 10s for requests for all requests that expect a response except for those initiated with
.fetch(which remains at 10s).