From b8a1525b7fb13b63ff934c5df0ac776659910c4a Mon Sep 17 00:00:00 2001 From: anthony-lopez-pd Date: Sun, 6 Sep 2026 10:43:06 -0400 Subject: [PATCH 1/3] fix: stop GenericTcpIpClient opening a duplicate socket on every connect Connect() stopped the retry timer and then re-armed it two lines later: RetryTimer.Stop(); _client = new TCPClient(Hostname, Port, BufferSize); ... RetryTimer.Reset(); // <-- introduced in v1.4.3 _client.ConnectToServerAsync(ConnectToServerCallback); RetryTimer's callback is Reconnect(), so every deliberate connect also scheduled a second connect one AutoReconnectIntervalMs later. Reconnect() does guard on IsConnected, but ConnectToServerAsync is still in flight at that point so IsConnected is false and it proceeds, opening a second socket. The comment directly above ("Stop retry timer if running") shows the intent. Measured on a bench against three PJLink projectors, v1.4.3 vs v1.3.2: v1.3.2 2856 connects, 0 idle-timeout closes 3 sockets per poll cycle v1.4.3 24 connects, 9 idle-timeout closes 6 sockets per poll cycle 38% of connections were opened, never used, and force-closed by the projector on its 30s idle timeout (PJLink spec v1.04 5.4). This matters beyond wasted sockets: PJLink devices commonly cap concurrent connections, so doubling the count risks the controller being refused. Removing the Reset() does not affect genuine reconnection. The retry timer is still armed in WaitAndTryReconnect(), which is called from both failure paths - ConnectToServerCallback() when a connect attempt fails, and Client_SocketStatusChange() on any non-connected status. Co-Authored-By: Claude Opus 5 (1M context) --- src/Comm/GenericTcpIpClient.cs | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/src/Comm/GenericTcpIpClient.cs b/src/Comm/GenericTcpIpClient.cs index 489adaf..4fbd7b3 100644 --- a/src/Comm/GenericTcpIpClient.cs +++ b/src/Comm/GenericTcpIpClient.cs @@ -318,7 +318,15 @@ public void Connect() _client.SocketStatusChange -= Client_SocketStatusChange; _client.SocketStatusChange += Client_SocketStatusChange; DisconnectCalledByUser = false; - RetryTimer.Reset(); + // NOTE: RetryTimer must NOT be armed here. Doing so schedules Reconnect() + // one AutoReconnectIntervalMs after every deliberate connect, and because + // ConnectToServerAsync is still in flight at that point IsConnected is + // still false, so Reconnect() proceeds and opens a SECOND socket. + // Measured on a bench against three PJLink projectors: 6 sockets per poll + // cycle instead of 3, with 38% of connections opened, never used, and + // force-closed by the projector on its 30s idle timeout. + // The retry timer is armed where it belongs - in the failure paths + // (ConnectToServerCallback / Client_SocketStatusChange -> WaitAndTryReconnect). _client.ConnectToServerAsync(ConnectToServerCallback); } } From 88fefc2093c134d60db192b3194aa0f1e20ec0e1 Mon Sep 17 00:00:00 2001 From: anthony-lopez-pd Date: Sun, 6 Sep 2026 10:57:20 -0400 Subject: [PATCH 2/3] fix: dispose the TCPClient instead of orphaning it on every connect Connect() replaced _client with a new TCPClient on every call and the previous instance was never disposed: _client = new TCPClient(Hostname, Port, BufferSize); DisconnectClient() only calls DisconnectFromServer(), which closes the connection but does not release the object. Crestron's TCPClient is disposable - the SDK documents TCPClient.Dispose as "free resources and disconnect" - and it holds unmanaged socket resources, so disconnecting without disposing leaves those resources outstanding. This matters because the controller reconnects constantly rather than holding a session. PJLink spec v1.04 5.4 requires the controller to close as soon as its command batch completes, so a bench with three projectors measured ~360 connects/hour. Every one of those orphaned a TCPClient. Adds DisposeClient(), which unsubscribes the socket status handler, disconnects if still connected, disposes, and nulls the field. It is called from two places: - Connect(), before allocating the replacement - Deactivate(), which previously unsubscribed and disconnected but left the object undisposed Also null-guards the Connected property. It dereferenced _client without a check, which was already an NPE waiting for any caller that read it before the first connect, and is now reachable because _client is nulled on dispose. IsConnected and ClientStatus were already guarded. Exceptions during dispose are caught and logged - a socket the platform has already torn down can throw, and that must never block a reconnect. Co-Authored-By: Claude Opus 5 (1M context) --- src/Comm/GenericTcpIpClient.cs | 42 +++++++++++++++++++++++++++++----- 1 file changed, 36 insertions(+), 6 deletions(-) diff --git a/src/Comm/GenericTcpIpClient.cs b/src/Comm/GenericTcpIpClient.cs index 4fbd7b3..ce4aa60 100644 --- a/src/Comm/GenericTcpIpClient.cs +++ b/src/Comm/GenericTcpIpClient.cs @@ -165,7 +165,7 @@ public ushort UAutoReconnect /// public bool Connected { - get { return _client.ClientStatus == SocketStatus.SOCKET_STATUS_CONNECTED; } + get { return _client != null && _client.ClientStatus == SocketStatus.SOCKET_STATUS_CONNECTED; } } //Lock object to prevent simulatneous connect/disconnect operations @@ -276,11 +276,7 @@ public override bool Deactivate() { RetryTimer.Stop(); RetryTimer.Dispose(); - if (_client != null) - { - _client.SocketStatusChange -= this.Client_SocketStatusChange; - DisconnectClient(); - } + DisposeClient(); return true; } @@ -314,6 +310,12 @@ public void Connect() Debug.Console(1, this, "Creating new TCPClient"); //Stop retry timer if running RetryTimer.Stop(); + // Release the previous socket before replacing it. TCPClient is + // IDisposable ("free resources and disconnect") and holds unmanaged + // socket resources; without this, every connect orphaned a live + // TCPClient. Because the controller reconnects on every poll cycle, + // that is a per-connection leak rather than a one-off. + DisposeClient(); _client = new TCPClient(Hostname, Port, BufferSize); _client.SocketStatusChange -= Client_SocketStatusChange; _client.SocketStatusChange += Client_SocketStatusChange; @@ -394,6 +396,34 @@ public void DisconnectClient() } } + /// + /// Unsubscribes from the current socket's events and disposes it. + /// Safe to call when no client exists. + /// + private void DisposeClient() + { + if (_client == null) + return; + + try + { + _client.SocketStatusChange -= Client_SocketStatusChange; + if (IsConnected) + _client.DisconnectFromServer(); + _client.Dispose(); + } + catch (Exception ex) + { + // 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); + } + finally + { + _client = null; + } + } + /// /// Callback method for connection attempt /// From c3f7bf3a3bf0b8cf140880cc3d942a197c46f8bc Mon Sep 17 00:00:00 2001 From: anthony-lopez-pd Date: Mon, 7 Sep 2026 07:33:25 -0400 Subject: [PATCH 3/3] =?UTF-8?q?fix:=20address=20review=20=E2=80=94=20lock?= =?UTF-8?q?=20Deactivate,=20avoid=20stale=20=5Fclient=20in=20status=20call?= =?UTF-8?q?back?= 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;