Conversation
🦋 Changeset detectedLatest commit: e8eea3b The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
|
1egoman
left a comment
There was a problem hiding this comment.
Thanks for the PR - this looks like a really promising new addition. Left a few small comments, other than those this looks good to me.
| } | ||
| if (alreadyAborted) return; | ||
|
|
There was a problem hiding this comment.
question: Are there other places further down in this method where something like this could be needed as well? What happens if an abort gets triggered midway through the signaling websocket connection process?
| * having given up on reconnecting. | ||
| */ | ||
| async close(reason?: string) { | ||
| this.reconnectAbortController?.abort(); |
There was a problem hiding this comment.
After aborting this here, I think it might be a good idea to reset this.reconnectAbortController back to its initial undefined value (like you are doing in the finally in attemptReconnect). While in practice I think this isn't critical for the change today, it means that any future code will be able to use the existence of this.reconnectAbortController as a meaningful signal.
| this.clearPendingReconnect(); | ||
| succeeded = true; | ||
| } catch (e) { | ||
| if (abortController.signal.aborted) return; |
There was a problem hiding this comment.
For all of these types of bail outs throughout the change, I wonder if it would be a good idea to add some sort of log for each one so that a user debugging a connection failure would be able to tell a) there was a purposeful bailout midway through, and b) which one bailout was taken. Thoughts on this?
|
Ah @horvatz would you be able to click the button in this comment and sign the CLA? That is a prerequisite before this could be merged. |
Calling
Room.disconnect()while a full reconnect is in progress can leave the reconnect coroutine running after the engine has closed. When paused cleanup or region selection finishes, it can calljoinwith the saved credentials. A late signal join response can also set_isClosedback tofalse. By then engine close has removed its listeners, so applications cannot reliably catch the late attempt through room-state events.This change gives each reconnect attempt an abort controller, aborts it synchronously on engine close, and passes its signal through full reconnect joins and region selection. Cancelled continuations stop without retrying, and late join responses cannot reopen the engine. SignalClient also checks cancellation after waiting for its connection lock/connection parameters and after previous-transport teardown, before creating another transport.
Reproduction and regression coverage
The original sequence is: start a full reconnect, pause peer-connection cleanup, await
Room.disconnect(), then release cleanup. Before this change a late join is attempted; after it no join is attempted and the room/engine remain disconnected/closed.Six new regression cases failed before the corresponding fixes and pass afterward:
A successful full reconnect is covered as well. Tests exercise the actual engine reconnect/join and signal-client paths with network/peer-connection work stubbed; no live server is required. The original reproduction was also rerun through real
Room.disconnect().Validation