From c3f7bf3a3bf0b8cf140880cc3d942a197c46f8bc Mon Sep 17 00:00:00 2001 From: anthony-lopez-pd Date: Mon, 7 Sep 2026 07:33:25 -0400 Subject: [PATCH] =?UTF-8?q?fix:=20address=20review=20=E2=80=94=20lock=20De?= =?UTF-8?q?activate,=20avoid=20stale=20=5Fclient=20in=20status=20callback?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three review comments on the disposal change, all valid. 1. Deactivate() disposed RetryTimer and the client without holding connectLock, so it could race Connect()/Disconnect()/Reconnect() and WaitAndTryReconnect's lock-protected RetryTimer.Reset. Now wrapped in connectLock/try-finally like the other lifecycle methods. Verified no same-thread re-entry: nothing DisposeClient() calls takes the lock. 2. Client_SocketStatusChange's connected path called _client.ReceiveDataAsync(...) on the field rather than the callback's own TCPClient. Since DisposeClient() now nulls _client, a queued status change could arrive afterwards and throw. It now uses the `client` argument, guarded with a null check and a ReferenceEquals test so events from a superseded socket don't re-arm receive on a dead client. The ConnectionChange event still fires as before — the guard only skips the receive re-arm, not the notification. 3. The dispose catch logged ex.Message, dropping the stack trace and any inner exception. Now logs the full exception, which is exactly what a disposal race needs. Co-Authored-By: Claude Opus 5 (1M context) --- src/Comm/GenericTcpIpClient.cs | 35 +++++++++++++++++++++++++++++----- 1 file changed, 30 insertions(+), 5 deletions(-) diff --git a/src/Comm/GenericTcpIpClient.cs b/src/Comm/GenericTcpIpClient.cs index ce4aa60..0beed55 100644 --- a/src/Comm/GenericTcpIpClient.cs +++ b/src/Comm/GenericTcpIpClient.cs @@ -274,9 +274,21 @@ void CrestronEnvironment_EthernetEventHandler(EthernetEventArgs ethernetEventArg /// public override bool Deactivate() { - RetryTimer.Stop(); - RetryTimer.Dispose(); - DisposeClient(); + // Teardown must hold connectLock like the other lifecycle methods. Without it, + // Deactivate can race Connect()/Disconnect()/Reconnect() and WaitAndTryReconnect's + // lock-protected RetryTimer.Reset — disposing the timer or nulling _client while + // another thread is mid-use. + try + { + connectLock.Enter(); + RetryTimer.Stop(); + RetryTimer.Dispose(); + DisposeClient(); + } + finally + { + connectLock.Leave(); + } return true; } @@ -416,7 +428,9 @@ private void DisposeClient() { // Disposing a socket that the platform has already torn down can throw. // That must never prevent a reconnect. - Debug.Console(1, this, "Exception disposing client: {0}", ex.Message); + // Log the full exception, not just Message — a disposal race is exactly the case + // where the stack trace and any inner exception are what you need. + Debug.Console(1, this, "Exception disposing client: {0}", ex); } finally { @@ -557,7 +571,18 @@ void Client_SocketStatusChange(TCPClient client, SocketStatus clientSocketStatus { Debug.Console(1, this, "Socket status change {0} ({1})", clientSocketStatus, ClientStatusText); RetryTimer.Stop(); - _client.ReceiveDataAsync(Receive); + // Use the callback's own client rather than the _client field. A queued status + // change can be delivered after DisposeClient() has nulled or replaced _client, + // which would throw here. Also ignore events from a socket that has already been + // superseded — re-arming receive on a dead socket is pointless. + if (client != null && ReferenceEquals(client, _client)) + { + client.ReceiveDataAsync(Receive); + } + else + { + Debug.Console(1, this, "Status change from a superseded socket; not re-arming receive"); + } } var handler = ConnectionChange;