fix: GenericTcpIpClient opens a duplicate socket per connect and never disposes it

[anthony-lopez-pd]

Two independent defects in GenericTcpIpClient, both found while chasing a memory leak on a
site running three PJLink projectors, and both verified on hardware.

1. Duplicate socket on every connect (regression in v1.4.3)

Connect() stops the retry timer and then re-arms it two lines later:

RetryTimer.Stop();
_client = new TCPClient(Hostname, Port, BufferSize);
...
RetryTimer.Reset();                 // added 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() guards on IsConnected, but
ConnectToServerAsync is still in flight at that point, so it proceeds and opens a second
socket. The comment directly above (//Stop retry timer if running) shows the intent.

Removing it does not affect genuine reconnection — the retry timer is still armed in
WaitAndTryReconnect(), called from both failure paths (ConnectToServerCallback on a failed
attempt, Client_SocketStatusChange on any non-connected status).

2. TCPClient is never disposed

Connect() replaced _client with a new TCPClient on every call and the old instance was
never released. DisconnectClient() only calls DisconnectFromServer(), which closes the
connection but does not free the object. The SDK documents TCPClient.Dispose as
"free resources and disconnect", and it holds unmanaged socket resources.

This matters because controllers reconnect constantly rather than holding a session — PJLink
spec v1.04 §5.4 requires the controller to close as soon as its command batch completes, which
measured ~360 connects/hour on this bench. Every one orphaned a TCPClient.

Adds DisposeClient() (unsubscribe, disconnect if connected, dispose, null), called from
Connect() before allocating the replacement and from Deactivate(). Exceptions during
dispose are caught and logged so a socket the platform has already torn down can never block a
reconnect.

Also null-guards the Connected property, which dereferenced _client unchecked — already an
NPE for any caller reading it before the first connect, and now reachable since _client is
nulled on dispose. IsConnected and ClientStatus were already guarded.

Measured on hardware

Three PJLink projectors, 2-second poll, prerelease .clz built by CI on each branch.

	v1.3.2	v1.4.3	fix 1	fix 1 + 2
Sockets per poll cycle	3	6	3	3
Connections never used	0	9 of 24 (38%)	0 of 32	0 of 30
Auth timeouts (unused)	0	9	0	0
Idle timeouts (used)	0	0	3	0
Commands per connection	5	mixed	1,1,1,3,5..9	all 5

With both fixes, all 30 sampled connections authenticated, carried exactly their 5-command
batch, and closed cleanly.

Memory, same rig, warm-up excluded, windows of 12h+:

Build	vsz	rss
v1.3.2	+1.451 MB/h	+1.364 MB/h
v1.4.3	+0.289 MB/h	+0.260 MB/h

v1.4.3 alone cut the leak ~81% and the memory now plateaus after ~4 hours. A soak on this
branch is in progress to quantify the disposal contribution; the remaining background rate
measured with the client idle is ~0.16 MB/h.

Note

Defect 1 is a regression in shipped v1.4.3 and will affect anyone using GenericTcpIpClient
against a device that reconnects frequently.

[Copilot]

🟡 Changes recommended

The new disposal/nulling of _client introduces concurrency risk (notably in socket status callbacks) and Deactivate() should be synchronized with the existing connectLock pattern to prevent races/use-after-dispose.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR fixes connection-lifecycle issues in GenericTcpIpClient that caused (1) a duplicate TCP socket to be opened during deliberate connects due to erroneously re-arming the reconnect timer, and (2) unmanaged socket resources to leak because replaced TCPClient instances were never disposed.

Changes:

• Adds safe client teardown via a new DisposeClient() helper, and uses it when reconnecting/replacing the socket and during Deactivate().
• Removes the reconnect-timer re-arming during Connect() to prevent an unintended second concurrent connect attempt.
• Null-guards the Connected property to avoid dereferencing _client before the first connection / after disposal.

File summaries

File	Description
src/Comm/GenericTcpIpClient.cs	Prevents duplicate connects by not re-arming the reconnect timer in Connect(), and disposes old TCPClient instances to avoid socket/resource leaks.

Review details

• Files reviewed: 1/1 changed files
• Comments generated: 3
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.