Skip to content

Conversation

@pblazej
Copy link
Contributor

@pblazej pblazej commented Aug 20, 2025

Resolves #757

  • Adds cancellation to reconnect, making sure that it cannot continue after disconnect() and bring the room back to life
  • ⚠️ Optionally, we can introduce a breaking .disconnecting state to prevent new tasks from being started (vs cancelling ongoing tasks - a little less elegant)
    • Theoretically it should not affect other state transitions, as disconnect() is basically "destructive" - the only valid path leads to .disconnected during cleanup(), however that's not statically enforced in any way
    • On the other hand, it does not provide any added value for other parts of the SDK, just covering this tiny time window

@github-actions
Copy link

github-actions bot commented Aug 20, 2025

⚠️ This PR does not contain any files in the .changes directory.

@saarva
Copy link

saarva commented Aug 25, 2025

@pblazej this fixed some disconnect issues that we were seeing, do you know if it'll go into the main branch soon?

@pblazej
Copy link
Contributor Author

pblazej commented Aug 26, 2025

@pblazej this fixed some disconnect issues that we were seeing, do you know if it'll go into the main branch soon?

Should be merged soon, the risk is moderate. We need to finalize the ongoing discussion about catching more edge cases (see the issue).

@pblazej pblazej marked this pull request as ready for review August 26, 2025 08:08
@pblazej pblazej requested a review from hiroshihorie August 26, 2025 08:08
Copy link
Member

@hiroshihorie hiroshihorie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but If possible, I would rather avoid exposing the .disconnecting connection state for developer experience / simplicity. Removing/deprecating this would be hard in the future.

@pblazej
Copy link
Contributor Author

pblazej commented Sep 22, 2025

LGTM but If possible, I would rather avoid exposing the .disconnecting connection state for developer experience / simplicity. Removing/deprecating this would be hard in the future.

  • For the current (major) version - yes, that's a bit cumbersome to introduce it atm
  • For the next version - why do we wanna remove that? I think it's a good addition to the (incomplete) state machine, without that you're basically unable to resolve the root cause of this one completely ⬆️ tl;dr now it looks complete (with all async state transitions)

@hiroshihorie I'm fine with reverting the state right now, if we're able to retest it.

@pblazej
Copy link
Contributor Author

pblazej commented Sep 22, 2025

stateDiagram-v2
    [*] --> Disconnected

    Disconnected --> Connecting: start connection
    Connecting --> Connected: success
    Connecting --> Disconnected: failed/canceled

    Connected --> Disconnecting: user closes
    Disconnecting --> Disconnected: closed

    Connected --> Reconnecting: network loss / ICE restart
    Reconnecting --> Connected: recovered
    Reconnecting --> Disconnected: failed to reconnect
    Reconnecting --> Reconnecting: retrying
Loading

@pblazej pblazej merged commit 2dff80b into main Sep 24, 2025
29 of 33 checks passed
@pblazej pblazej deleted the blaze/disconnect-reconnect branch September 24, 2025 06:15
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.

Disconnect is unreliable

4 participants