GenericTcpIpClient never disposes its TCPClient on main (4-Series 2.x) — fixed on maintenance-1x in v1.4.4

[anthony-lopez-pd]

Summary

GenericTcpIpClient never disposes its TCPClient. Every call to Connect() replaces the
_client field with a new instance, and the previous one is only ever disconnected — never
freed. Against a device that reconnects frequently, each connect orphans a live socket object
holding unmanaged resources, and the hosting process grows without bound.

This was found and fixed on the 1.x maintenance line (maintenance-1x), released in
v1.4.4 via #198 with review follow-ups in #199. The same defect is present on main
(the 4-Series 2.x line) and is unfixed there.

Evidence on main

src/Comm/GenericTcpIpClient.cs, in Connect():

//Stop retry timer if running
RetryTimer.Stop();
_client = new TCPClient(Hostname, Port, BufferSize);   // <-- previous _client leaked
_client.SocketStatusChange -= Client_SocketStatusChange;
_client.SocketStatusChange += Client_SocketStatusChange;
DisconnectCalledByUser = false;
_client.ConnectToServerAsync(ConnectToServerCallback);

There are 24 references to _client in the file and not one .Dispose() on it.
Deactivate() disposes RetryTimer and unsubscribes the socket-status handler, but leaves the
socket itself:

public override bool Deactivate()
{
    RetryTimer.Stop();
    RetryTimer.Dispose();
    if (_client != null)
    {
        _client.SocketStatusChange -= this.Client_SocketStatusChange;
        DisconnectClient();          // disconnects, does not dispose
    }
    return true;
}

DisconnectClient() calls DisconnectFromServer(). A disconnected TCPClient is still a live
object holding a socket handle; it needs disposing.

Measured impact on the 1.x line

Found while investigating firmware-forced low-memory reboots on 4-Series processors. The device
was a PJLink projector, which mandates that the controller close the connection after each
command batch (spec §5.4) and times out an idle socket after 30 s — so the client reconnected
roughly 360 times per hour, per device.

Slopes measured on hardware, warm-up hour excluded from each run:

Build	Window	vsz	rss
before both fixes	11.9 h	+1.451 MB/h	+1.364 MB/h
after fixing the duplicate connect only	14.0 h	+0.288 MB/h	+0.259 MB/h
after adding disposal	14.0 h	−0.008 MB/h	−0.015 MB/h
released v1.4.4, re-confirmed	3.3 h	−0.072 MB/h	−0.114 MB/h

Background rate with traffic stopped entirely was ~0.16 MB/h, so the final build is flat inside
the noise floor. Time to the low-memory threshold went from 3–10 days to no trend at all.

The leak is proportional to connect frequency, so a long-lived connection will not show it. Any
consumer that reconnects — a device that closes its own sockets, a polling client, anything
behind an unreliable link — will.

What does NOT apply to main

For completeness, #198 fixed two defects. Only one of them is relevant here:

• Duplicate socket per connect — Connect() calling RetryTimer.Reset() immediately after
  stopping it, scheduling a second connect after every deliberate one. Not present on
  main — Connect() there has only RetryTimer.Stop(). That was a regression introduced in
  shipped v1.4.3 on the maintenance line.
• Socket never disposed — this issue. Present on both lines.

Suggested fix

Port the disposal path from maintenance-1x. The shape that shipped:

private void DisposeClient()
{
    if (_client == null) return;
    try
    {
        _client.SocketStatusChange -= Client_SocketStatusChange;
        if (IsConnected) _client.DisconnectFromServer();
        _client.Dispose();
    }
    catch (Exception ex)
    {
        // Disposing a socket the platform has already torn down can throw.
        // That must never prevent a reconnect.
        Debug.Console(1, this, "Exception disposing client: {0}", ex);
    }
    finally { _client = null; }
}

called from Connect() before the new instance is created, and from Deactivate() in place of
DisconnectClient().

Two review findings from #199 are worth carrying across at the same time, because they are
consequences of DisposeClient() nulling a shared field:

1. Client_SocketStatusChange must use the callback's own client argument, not the _client
   field. A queued status change can be delivered after _client has been nulled or replaced,
   which throws on re-arm. main currently has the same pattern:

   _client.ReceiveDataAsync(Receive);   // stale field

   The shipped fix re-arms only when the event's socket is still current:

   if (client != null && ReferenceEquals(client, _client)) { client.ReceiveDataAsync(Receive); }

   Note it deliberately does not early-return, so ConnectionChange still fires for superseded
   sockets.

2. Deactivate() should hold connectLock like the other lifecycle methods, or teardown can
   race Connect()/Disconnect()/Reconnect() and dispose the timer or null _client while
   another thread is mid-use.

Verification notes

If you reproduce this, the socket count is a faster signal than the memory slope: against a
device that closes its own connections, count connections per poll cycle and how many are opened
but never used. Unfixed, we measured 9 of 24 connections opened, never used, and force-closed by
the peer. Fixed: 0 unused across 6,882 connections.

Commits on the 1.x line, for reference: 88fefc2 (disposal), c3f7bf3 (the two review fixes).