Skip to content

fix: prevent identity mismatch on reconnect during in-flight retrieveId #1362

Open
dearlordylord wants to merge 1 commit into
peers:masterfrom
dearlordylord:fix/identity-mismatch-on-reconnect
Open

fix: prevent identity mismatch on reconnect during in-flight retrieveId #1362
dearlordylord wants to merge 1 commit into
peers:masterfrom
dearlordylord:fix/identity-mismatch-on-reconnect

Conversation

@dearlordylord

Copy link
Copy Markdown
  • new Peer() (no id) → disconnect()reconnect() while retrieveId() is in-flight creates a WebSocket registered with a null ID on the server, causing all signaling to silently fail

  • Three independent root causes fixed:

  • Guard _initialize(): late retrieveId() resolve no longer runs on a destroyed/disconnected peer

  • AbortController: in-flight retrieveId() fetch is cancelled on disconnect() via AbortSignal

  • Null check in reconnect(): emits error instead of opening a socket with null ID when _lastServerId was never assigned

seems to be partially related to #1289

new Peer() → disconnect() → reconnect() while retrieveId() is in-flight
creates a WebSocket registered with a null ID on the server, causing all
signaling to silently fail.

Three independent fixes:
- Guard _initialize() against destroyed/disconnected state
- Abort in-flight retrieveId() fetch on disconnect via AbortController
- Reject reconnect() when _lastServerId is null (no ID was ever assigned)
@mrmoon0986

Copy link
Copy Markdown

Sharp catch — the in-flight retrieveId vs disconnect race is exactly the kind of bug that fails silently and eats days. Cancelling the fetch with an AbortController and guarding _initialize() against a destroyed peer is the right move, and the null _lastServerId path opening a socket with a null ID is especially nasty since nothing throws.

Looks like it's been sitting without a review since March though. Is peerjs-server still maintained from your side, or are you running a fork? Curious how you ended up handling reconnect identity in production given the repo's pace.

@dearlordylord

Copy link
Copy Markdown
Author

Yes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants