diff --git a/README.md b/README.md index 284dee4c..ed503eec 100644 --- a/README.md +++ b/README.md @@ -208,6 +208,8 @@ IEnumerator ConnectToRoom() } ``` +Subscribe to `room.Disconnected` (or `room.DisconnectedWithReason`) for your teardown: it is raised for server-side disconnects and, with `DisconnectReason.ClientInitiated`, for your own `room.Disconnect()` or `Dispose()` as well, so one handler covers both. Handlers run before the room's handles are released. `room.ConnectionStateChanged` reports the same transitions, and `room.IsConnected` is true from the moment `Connected` is raised. One gap to know: a `Disconnect()` while the connect is still pending is a no-op, so disconnect again once `Connect` has completed. + ### Video #### Publishing a texture (e.g Unity Camera) diff --git a/Runtime/Scripts/Core/Room.cs b/Runtime/Scripts/Core/Room.cs index 44d52302..e74f80e0 100644 --- a/Runtime/Scripts/Core/Room.cs +++ b/Runtime/Scripts/Core/Room.cs @@ -112,6 +112,9 @@ public Proto.RoomOptions ToProto() public class Room : IDisposable { internal FfiHandle RoomHandle = null; + // Set at the start of Teardown, before the disconnect is reported, so a handler + // that calls Disconnect() re-entrantly is a no-op; reset by OnConnect so a Room + // can be connected again after a disconnect. private bool _disposed = false; private readonly Dictionary _participants = new(); private StreamHandlerRegistry _streamHandlers = new(); @@ -190,20 +193,22 @@ public ConnectInstruction Connect(string url, string token, RoomOptions options) return instruction; } + /// + /// Disconnects from the room. A local disconnect is reported the way a + /// server-side one is — , + /// and with + /// , synchronously and with the + /// room's handles still live — so one teardown path covers both. + /// goes through here too. Calling this from a handler of + /// those events, or on a room that already disconnected, is a no-op. So is a + /// call while is still pending: that connect completes, + /// and the app has to disconnect again once it has. + /// public void Disconnect() { if (_disposed || RoomHandle == null) return; - var (response, _) = FFIBridge.Instance.SendDisconnectRequest(this); - using (response) - { - Utils.Debug($"Disconnect.... {RoomHandle}"); - Utils.Debug($"Disconnect response.... {response}"); - } - // Release the Rust-side room synchronously. Without this the FfiRoom - // (peer connection, signaling client, libwebrtc state) lingers in the - // FFI handle table until the SafeHandle finalizer runs. - Cleanup(); + Teardown(DisconnectReason.ClientInitiated, closeFfiRoom: true); } public void Dispose() @@ -212,20 +217,67 @@ public void Dispose() GC.SuppressFinalize(this); } - private void Cleanup() + // The single exit for every disconnect path — a local Disconnect(), the core's + // Disconnected event, a panic. Reports the disconnect once, then releases the + // room: handlers run first, with the handles still live, as they do on a + // server-side disconnect, and the release runs in a finally so a throwing + // handler cannot leave the Rust-side room alive. closeFfiRoom asks the Rust side + // to close the room (local path); on the other paths the core already has. + private void Teardown(DisconnectReason reason, bool closeFfiRoom) { if (_disposed) return; _disposed = true; + // Unsubscribed first: nothing still queued for this room — including the + // core's own Disconnected event for a local close — is processed from here on. FfiClient.Instance.RoomEventReceived -= OnEventReceived; FfiClient.Instance.RpcMethodInvocationReceived -= OnRpcMethodInvocationReceived; FfiClient.Instance.DisconnectReceived -= OnDisconnectReceived; FfiClient.Instance.PanicReceived -= OnPanicReceived; - // Participant + track + publication FFI handles are independent entries in the - // Rust handle table — dropping the room handle alone does not cascade to them, so - // they would otherwise linger until C# GC finalizes each SafeHandle. + try + { + DisconnectReason = reason; + SetConnectionState(ConnectionState.ConnDisconnected); + Disconnected?.Invoke(this); + DisconnectedWithReason?.Invoke(this, reason); + } + finally + { + try + { + if (closeFfiRoom) + CloseFfiRoom(); + } + finally + { + ReleaseHandles(); + } + } + } + + // Sent after the handlers on purpose: the Rust-side close replaces the remote + // track handles on its worker thread right away, so a handler touching tracks + // would otherwise race it. + private void CloseFfiRoom() + { + var (response, _) = FFIBridge.Instance.SendDisconnectRequest(this); + using (response) + { + Utils.Debug($"Disconnect.... {RoomHandle}"); + Utils.Debug($"Disconnect response.... {response}"); + } + } + + // Released synchronously. Without this the FfiRoom (peer connection, signaling + // client, libwebrtc state) lingers in the FFI handle table until the SafeHandle + // finalizer runs. Participant + track + publication FFI handles are independent + // entries in the Rust handle table — dropping the room handle alone does not + // cascade to them, so they would otherwise linger until C# GC finalizes each + // SafeHandle. + private void ReleaseHandles() + { LocalParticipant?.DisposeHandles(); foreach (var p in _participants.Values) p.DisposeHandles(); @@ -235,6 +287,19 @@ private void Cleanup() RoomHandle = null; } + // Records a connection-state transition and raises ConnectionStateChanged for it. + // A repeat of the current state is dropped: the core's own ConnectionStateChanged + // (Connected) is queued behind the connect callback and drains one event pass + // after OnConnect recorded the transition, so this check is what keeps every + // connect to a single Connected report. + private void SetConnectionState(ConnectionState state) + { + if (ConnectionState == state) + return; + ConnectionState = state; + ConnectionStateChanged?.Invoke(state); + } + /// /// Registers a handler for incoming text streams matching the given topic. /// @@ -308,7 +373,7 @@ internal void OnRpcMethodInvocationReceived(RpcMethodInvocationEvent e) internal void OnEventReceived(RoomEvent e) { - // After Cleanup() the handle is null but late events may still flow + // After Teardown the handle is null but late events may still flow // through the FfiClient before the unsubscribe fully takes effect. if (RoomHandle == null) return; @@ -542,14 +607,17 @@ internal void OnEventReceived(RoomEvent e) } break; case RoomEvent.MessageOneofCase.ConnectionStateChanged: - ConnectionState = e.ConnectionStateChanged.State; - ConnectionStateChanged?.Invoke(e.ConnectionStateChanged.State); + // The core reports a disconnect as ConnectionStateChanged(Disconnected) + // followed by Disconnected{reason}. The transition is recorded from + // the latter, once the reason is known, so handlers of either event + // see the same DisconnectReason on every path, and a handler that + // disconnects in between cannot replace the core's reason with + // ClientInitiated. + if (e.ConnectionStateChanged.State != ConnectionState.ConnDisconnected) + SetConnectionState(e.ConnectionStateChanged.State); break; case RoomEvent.MessageOneofCase.Disconnected: - DisconnectReason = e.Disconnected.Reason; - Disconnected?.Invoke(this); - DisconnectedWithReason?.Invoke(this, DisconnectReason); - OnDisconnect(); + Teardown(e.Disconnected.Reason, closeFfiRoom: false); break; case RoomEvent.MessageOneofCase.Reconnecting: Reconnecting?.Invoke(this); @@ -590,6 +658,9 @@ internal void OnEventReceived(RoomEvent e) internal void OnConnect(ConnectCallback info) { RoomHandle = FfiHandle.FromOwnedHandle(info.Result.Room.Handle); + // A Room may be connected again after a disconnect: each connect starts a + // fresh lifecycle. + _disposed = false; UpdateFromInfo(info.Result.Room.Info); LocalParticipant = new LocalParticipant(info.Result.LocalParticipant, this); @@ -604,7 +675,7 @@ internal void OnConnect(ConnectCallback info) FfiClient.Instance.RpcMethodInvocationReceived += OnRpcMethodInvocationReceived; // Signal Rust that listeners are installed and it can start forwarding room events. - // Without this the FFI side parks for 1s after ConnectCallback and then drops the room + // Without this the FFI side parks for 15s after ConnectCallback and then drops the room // with ConnectionTimeout. Must run after the FfiClient.RoomEventReceived subscription // above so no event can race ahead of OnEventReceived. using (var readyRequest = FFIBridge.Instance.NewRequest()) @@ -613,6 +684,15 @@ internal void OnConnect(ConnectCallback info) using var readyResponse = readyRequest.Send(); } + // The core recorded this transition during connect, before this room was + // subscribed to its events (its own copy drains later and is dropped as a + // repeat, see SetConnectionState); recording it here makes IsConnected true + // from the moment Connected is raised. + SetConnectionState(ConnectionState.ConnConnected); + // A ConnectionStateChanged handler may already have disconnected the room; + // Connected is not raised on a torn-down room. + if (_disposed) + return; Connected?.Invoke(this); } @@ -628,15 +708,7 @@ private void OnPanicReceived(Panic e) // room could silently stop receiving events (including Disconnected // itself), so the panic is surfaced through the disconnect path apps // already handle. - DisconnectReason = DisconnectReason.UnknownReason; - Disconnected?.Invoke(this); - DisconnectedWithReason?.Invoke(this, DisconnectReason); - OnDisconnect(); - } - - private void OnDisconnect() - { - Cleanup(); + Teardown(DisconnectReason.UnknownReason, closeFfiRoom: false); } internal RemoteParticipant CreateRemoteParticipantWithTracks(ConnectCallback.Types.ParticipantWithTracks item) @@ -694,18 +766,26 @@ void OnConnect(ConnectCallback e) return; bool success = string.IsNullOrEmpty(e.Error); - if (success) + try { - if (_roomOptions.E2EE != null) + if (success) { - _room.E2EEManager = new E2EEManager(_room.RoomHandle, _roomOptions.E2EE); - } + if (_roomOptions.E2EE != null) + { + _room.E2EEManager = new E2EEManager(_room.RoomHandle, _roomOptions.E2EE); + } - _room.OnConnect(e); + _room.OnConnect(e); + } + } + finally + { + // Completed even when a ConnectionStateChanged or Connected handler throws + // inside OnConnect, so the code awaiting the connect resumes instead of + // hanging forever. + IsError = !success; + IsDone = true; } - - IsError = !success; - IsDone = true; } void OnCanceled() diff --git a/Samples~/Meet/Assets/Runtime/MeetManager.cs b/Samples~/Meet/Assets/Runtime/MeetManager.cs index 1dc27c71..ee8e91ac 100644 --- a/Samples~/Meet/Assets/Runtime/MeetManager.cs +++ b/Samples~/Meet/Assets/Runtime/MeetManager.cs @@ -142,17 +142,14 @@ private void Update() private void OnDestroy() { // Without this, scene change / app quit while connected leaves all tracks, - // streams, and their backing GPU/native resources allocated. - if (_room != null) - { - _room.Disconnect(); - _room = null; - } + // streams, and their backing GPU/native resources allocated. Disconnect reports + // back through OnDisconnected, which tears the call down; CleanUpAllTracks after + // it covers anything created without a room. + _room?.Disconnect(); CleanUpAllTracks(); _webCamTexture?.Stop(); _platformAudioSource?.Dispose(); _platformAudio?.Dispose(); - _room?.Disconnect(); } #endregion @@ -166,13 +163,9 @@ private void OnStartCall() private void OnEndCall() { - if (_room == null) return; - - _room.Disconnect(); - CleanUpAllTracks(); - _room = null; - _localId = null; - buttonBar.SetConnected(false); + // The SDK reports a local disconnect through OnDisconnected (with + // DisconnectReason.ClientInitiated), which owns the teardown. + _room?.Disconnect(); } private void OnToggleCamera() @@ -432,8 +425,20 @@ private void OnParticipantDisconnected(Participant participant, DisconnectReason DestroyParticipantTile(participant.Identity); } + // The one teardown path: raised for the hang-up button, a server-side disconnect + // (kick, room deleted, token expiry) and the scene going away (OnDestroy) alike, + // synchronously and with the room's handles still live. private void OnDisconnected(Room room) - => Debug.Log($"Disconnected from room: {room.DisconnectReason}"); + { + Debug.Log($"Disconnected from room: {room.DisconnectReason}"); + + CleanUpAllTracks(); + _room = null; + _localId = null; + // Destroyed already when this runs from OnDestroy during a scene unload. + if (buttonBar != null) + buttonBar.SetConnected(false); + } private void OnTrackMuted(TrackPublication publication, Participant participant) { diff --git a/Tests/EditMode/RoomDisconnectReasonTests.cs b/Tests/EditMode/RoomDisconnectReasonTests.cs index 11ed1ebc..046eed53 100644 --- a/Tests/EditMode/RoomDisconnectReasonTests.cs +++ b/Tests/EditMode/RoomDisconnectReasonTests.cs @@ -84,5 +84,54 @@ public void ParticipantDisconnected_SurfacesReasonOnEvent() Assert.IsNotNull(eventParticipant); Assert.AreEqual(identity, eventParticipant.Identity); } + + [Test] + public void Disconnected_ReentrantDisconnectFromHandlers_KeepsServerReason() + { + var room = new Room(); + room.RoomHandle = new FfiHandle(IntPtr.Zero); + // A fresh Room already reads ConnDisconnected (the enum default), so record a + // connected state first, as the core's own event would after a connect. + room.OnEventReceived(new RoomEvent + { + RoomHandle = 0, + ConnectionStateChanged = new ConnectionStateChanged { State = ConnectionState.ConnConnected } + }); + Assert.AreEqual(ConnectionState.ConnConnected, room.ConnectionState); + + var reports = 0; + DisconnectReason? reasonSeenByStateHandler = null; + room.ConnectionStateChanged += state => + { + if (state != ConnectionState.ConnDisconnected) return; + reasonSeenByStateHandler = room.DisconnectReason; + // An app that disconnects "to be sure" from its state handler. + room.Disconnect(); + }; + room.DisconnectedWithReason += (_, __) => + { + reports++; + room.Disconnect(); + }; + + // The core reports a server-side disconnect as two queued events. + room.OnEventReceived(new RoomEvent + { + RoomHandle = 0, + ConnectionStateChanged = new ConnectionStateChanged { State = ConnectionState.ConnDisconnected } + }); + room.OnEventReceived(new RoomEvent + { + RoomHandle = 0, + Disconnected = new Disconnected { Reason = DisconnectReason.ServerShutdown } + }); + + Assert.AreEqual(1, reports, "the disconnect is reported once"); + Assert.AreEqual(DisconnectReason.ServerShutdown, room.DisconnectReason, + "a re-entrant Disconnect() must not replace the server's reason with ClientInitiated"); + Assert.AreEqual(DisconnectReason.ServerShutdown, reasonSeenByStateHandler, + "the ConnectionStateChanged handler sees the reason already recorded"); + Assert.AreEqual(ConnectionState.ConnDisconnected, room.ConnectionState); + } } } diff --git a/Tests/PlayMode/RoomTests.cs b/Tests/PlayMode/RoomTests.cs index 669b0dc6..5cafba35 100644 --- a/Tests/PlayMode/RoomTests.cs +++ b/Tests/PlayMode/RoomTests.cs @@ -129,7 +129,7 @@ public IEnumerator RoomSid_StartsWithRM() StringAssert.StartsWith("RM_", context.Rooms[0].Sid); } - [UnityTest, Category("E2E"), Ignore("Known issue")] + [UnityTest, Category("E2E")] public IEnumerator ConnectionState_IsConnected() { using var context = new TestRoomContext(); @@ -232,7 +232,7 @@ public IEnumerator ParticipantDisconnect_TriggersEvent() if (expectation.Error != null) Assert.Fail(expectation.Error); } - [UnityTest, Category("E2E"), Ignore("Known issue")] + [UnityTest, Category("E2E")] public IEnumerator Disconnect_TriggersEvent() { using var context = new TestRoomContext(); @@ -267,5 +267,86 @@ public IEnumerator Disconnect_TriggersEvent() yield return expectation.Wait(); if (expectation.Error != null) Assert.Fail(expectation.Error); } + + [UnityTest, Category("E2E")] + public IEnumerator Connect_ReportsConnectedOnce() + { + using var context = new TestRoomContext(); + var room = context.Rooms[0]; + var connectedReports = 0; + room.ConnectionStateChanged += s => + { + if (s == ConnectionState.ConnConnected) connectedReports++; + }; + + yield return context.ConnectAll(); + Assert.IsNull(context.ConnectionError, context.ConnectionError); + Assert.AreEqual(1, connectedReports, "recorded from the connect callback"); + + // The core's own ConnectionStateChanged(Connected) is queued behind the connect + // callback and drains on a later frame; it must be absorbed as a repeat. + for (var i = 0; i < 5; i++) yield return null; + Assert.AreEqual(1, connectedReports, "the core's copy of the transition must not be reported again"); + } + + [UnityTest, Category("E2E")] + public IEnumerator Disconnect_ReportsClientInitiated_OnceWithRoomIntact() + { + using var context = new TestRoomContext(); + yield return context.ConnectAll(); + Assert.IsNull(context.ConnectionError, context.ConnectionError); + var room = context.Rooms[0]; + + var disconnected = 0; + var disconnectedWithReason = 0; + DisconnectReason? reason = null; + room.Disconnected += r => + { + disconnected++; + // Handlers run before the release, with the state already recorded. + Assert.IsNotNull(r.RoomHandle, "the room handle must still be live in the handler"); + Assert.IsFalse(r.LocalParticipant.Handle.IsClosed, "participant handles must still be live in the handler"); + Assert.AreEqual(ConnectionState.ConnDisconnected, r.ConnectionState); + Assert.AreEqual(DisconnectReason.ClientInitiated, r.DisconnectReason); + // A teardown handler that hangs up "to be sure" must not re-report. + r.Disconnect(); + }; + room.DisconnectedWithReason += (_, r) => + { + disconnectedWithReason++; + reason = r; + }; + + // Dispose is a disconnect too. Reported synchronously: no frame has to pass + // for a hang-up to be observable. + room.Dispose(); + + Assert.AreEqual(1, disconnected); + Assert.AreEqual(1, disconnectedWithReason); + Assert.AreEqual(DisconnectReason.ClientInitiated, reason); + Assert.AreEqual(ConnectionState.ConnDisconnected, room.ConnectionState); + Assert.IsNull(room.RoomHandle, "the room is released once the handlers ran"); + + // A later Disconnect (TestRoomContext.Dispose issues one too) reports nothing. + room.Disconnect(); + Assert.AreEqual(1, disconnected); + } + + [UnityTest, Category("E2E")] + public IEnumerator Disconnect_ThrowingHandler_StillReleasesRoom() + { + using var context = new TestRoomContext(); + yield return context.ConnectAll(); + Assert.IsNull(context.ConnectionError, context.ConnectionError); + var room = context.Rooms[0]; + + room.Disconnected += _ => throw new System.InvalidOperationException("handler failed"); + + Assert.Throws(() => room.Disconnect()); + + // The handler's exception surfaces, but the room is still released. + Assert.IsNull(room.RoomHandle, "the release must run even when a Disconnected handler throws"); + Assert.DoesNotThrow(() => room.Dispose()); + } } } \ No newline at end of file