Report client-initiated disconnects through Room.Disconnected - #382
Report client-initiated disconnects through Room.Disconnected#382MaxHeimbrock wants to merge 3 commits into
Conversation
A local Disconnect() or Dispose() now raises ConnectionStateChanged, Disconnected and DisconnectedWithReason with DisconnectReason.ClientInitiated, synchronously and before Cleanup releases the handles, as the other LiveKit SDKs and the Rust core do. Previously Cleanup unsubscribed the room from FFI events before the core's own Disconnected event could arrive, so apps needed two teardown paths. The report happens once per room across the remote event, panic and local paths, so a handler calling Disconnect() re-entrantly is a no-op. OnConnect records ConnConnected, which the core records before this wrapper subscribes to room events, so IsConnected is true on a connected room; repeats of the current state arriving from the core are dropped. Un-ignores the two RoomTests that covered these gaps and adds two more. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
All three disconnect paths (local Disconnect/Dispose, the core's Disconnected
event, panic) now exit through one Teardown(reason, closeFfiRoom): it marks the
room disposed, unsubscribes from FFI events, raises the events with the handles
still live, and releases the handles in a finally, so a throwing handler cannot
leave the Rust-side room alive. The local path sends the FFI disconnect request
after the handlers, because the Rust-side close replaces remote-track handles on
its worker thread immediately.
The core's ConnectionStateChanged(Disconnected) is recorded from the
Disconnected{reason} event that always follows it, so handlers of either event
see the same DisconnectReason on every path and a handler that disconnects in
between cannot replace the server's reason with ClientInitiated. OnConnect
resets the disposed flag so a Room can be connected again, and skips Connected
when a ConnectionStateChanged handler already disconnected. ConnectInstruction
completes in a finally so a throwing handler cannot hang the awaiting code.
Comments now describe the mechanism as measured (the core's Connected copy
always drains one pass later; the ready timeout is 15s). Tests pin the handles
being live inside the handler, one report per event, the repeat-drop, a
throwing handler, and a synthetic two-event server disconnect with re-entrant
handlers. The pending-connect Disconnect() no-op stays and is documented.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
With Room.Disconnect() reporting ClientInitiated through Disconnected, the sample no longer needs its own teardown behind the hang-up button: OnEndCall just disconnects, and OnDisconnected owns the teardown for the button, a server-side disconnect and OnDestroy alike. That also fixes the sample leaving tracks and the "connected" UI behind on a server-side disconnect, which it only logged before. OnDestroy loses its dead second Disconnect() call. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
| SetConnectionState(ConnectionState.ConnConnected); | ||
| // A ConnectionStateChanged handler may already have disconnected the room; | ||
| // Connected is not raised on a torn-down room. | ||
| if (_disposed) |
There was a problem hiding this comment.
if _disposed is true, while the connection state just gets set to ConnConnected, do they get out of sync then ?
Should we do some cleanup here ?
There was a problem hiding this comment.
The only way this case if (_disposed) is hit is when the handler registered to OnConnectionStateChanged is disconnecting the room, because all other calls between setting _disposed = false in line 663 and the SetConnectionState in line 691 are sync calls without any handlers.
If the handler on OnConnectionStateChanged would disconnect the room, then the ConnectionState will already be set to disconnected once line 694 is reached and the value is in sync with _disposed again. In that case the Connected event should be skipped, so the gate is correct here.
| _webCamTexture?.Stop(); | ||
| _platformAudioSource?.Dispose(); | ||
| _platformAudio?.Dispose(); | ||
| _room?.Disconnect(); |
There was a problem hiding this comment.
any reason to call _room?.Disconnect(); before shutting down the sources ?
and if _room?.Disconnect() is async, I wonder if we should wait until everything gets shutdown to clean up everything
There was a problem hiding this comment.
The removed _room?.Disconnect(); at the end was just a duplication. Usually the removed line 148 was calling _room.Disconnect() already at the top and it stays this way in the PR. I don't see an issue doing these things in parallel.
The room disconnect message should be independent from the other resources being torn down, except for PlatformAudio, which is safer to not dispose while the room is still active in my opinion.
2 participants
Background
Room.Disconnected/DisconnectedWithReasonwere only raised for server-side disconnects. A localDisconnect()(andDispose()) sent the FFI request and ran the cleanup, which unsubscribes the room from FFI events synchronously, so theClientInitiatedDisconnected event the Rust core emits duringclose()never reached the app.Related gap:
ConnectionStatenever leftConnDisconnectedafter a connect, because the Rust core records the Connected transition duringconnect, before this wrapper subscribes to room events.IsConnectedwas therefore false on a connected room. The two[Ignore("Known issue")]tests inRoomTestscovered exactly these two gaps, which are now enabled again: linkChanges
Room.Disconnect()andDispose()now raiseConnectionStateChanged(ConnDisconnected),DisconnectedandDisconnectedWithReasonwithDisconnectReason.ClientInitiated.All disconnect paths (local, core
Disconnectedevent, panic) share oneTeardown(reason, closeFfiRoom): mark disposed, unsubscribe from FFI events, raise events, release handles in afinally. Re-entrant or repeatedDisconnect()is a no-op.OnConnectrecordsConnConnectedand raisesConnectionStateChanged, soIsConnectedis true onceConnectedis raised. The core's own copy is deduplicated.OnConnectalso resets the disposed flag, so a reconnected Room gets a full lifecycle.ConnectInstructioncompletes in afinally, so a throwingOnConnecthandler no longer hangs the awaiting code.Meet sample:
OnEndCallonly callsDisconnect();OnDisconnectedowns the teardown for hang-up, server-side disconnect andOnDestroy. Server-side disconnects previously left tracks and the "connected" UI behind.Tests:
ConnectionState_IsConnectedandDisconnect_TriggersEventun-ignored. New PlayMode and EditMode tests cover the singleConnectedreport, the client-initiated disconnect (live handles, re-entrantDisconnect(),Dispose(), throwing handler) and re-entrant disconnects keeping the server reason.Behavior change for release notes
Every existing
Disconnectedhandler now also runs on the app's ownDisconnect()/Dispose(). A handler that is not idempotent with the code around the hang-up call will tear down twice. The Meet sample is updated to the single-handler shape in this PR; the Agents sample does not subscribe toDisconnectedand is unaffected.