fix(transport): dispose TcpClient/ClientWebSocket on failed connect in StreamFactory (fixes #216)#243
Merged
Merged
Conversation
…n StreamFactory (fixes #216) TCP and WebSocket client StreamFactories constructed a TcpClient/ClientWebSocket and immediately awaited ConnectAsync without wrapping the construct-through-connect span in try/dispose. Any throw from ConnectAsync (connection refused, host unreachable, DNS failure, TLS error, timeout, OperationCanceledException) leaked the just-allocated client: the underlying Socket was only released through finalization. Under the multiplexer reconnect loop targeting an unreachable endpoint, every retry leaked one FD/handle. UDP and QUIC factories in the same project already use the correct try/catch+Dispose pattern; TCP and WebSocket diverged. Fix (src/NetConduit.Transport.Tcp/TcpMultiplexer.cs, src/NetConduit.Transport.WebSocket/WebSocketMultiplexer.cs, src/NetConduit.Transport.WebSocket/WebSocketMuxListener.cs.CreateReconnectableClientOptions): - Wrap connect-through-return in try { ... } catch { client.Dispose(); throw; } mirroring UDP. - Applies to TcpMultiplexer.CreateOptions(string,int), CreateOptions(IPEndPoint), WebSocketMultiplexer.CreateOptions(Uri,...) (string overload delegates), and WebSocketMuxListener.CreateReconnectableClientOptions. Regression test (tests/NetConduit.Transport.Tcp.IntegrationTests/TcpMultiplexerFactoryLeakTests.cs): - Two tests cover both CreateOptions overloads. Each invokes the StreamFactory 50 times against RFC 5737 TEST-NET-1 (192.0.2.1:1) with a 50ms per-attempt CancellationToken, then asserts Process.HandleCount grew by < iterations/2. - Verified pre-fix: handle count grew by 55-57 over 50 iterations (1:1 leak ratio) — tests fail loudly. Post-fix: tests pass with negligible handle growth. Build: 0 warnings, 0 errors. Full sweep: 610 passed / 1 pre-existing skip / 0 failed (611 total, including the 2 new leak tests).
This was referenced May 21, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #216.
Problem
The TCP and WebSocket client
StreamFactoryimplementations constructed aTcpClient/ClientWebSocketand immediately awaitedConnectAsyncwithout wrapping the construct-through-connect span intry/Dispose. Any throw fromConnectAsync— connection refused, host unreachable, DNS failure, TLS error, timeout,OperationCanceledException— leaked the just-allocated client. The underlyingSocketwas only released through GC finalization.Under the multiplexer's reconnect loop pointed at an unreachable endpoint, every retry leaked one FD/handle, accumulating to socket / ephemeral-port exhaustion within minutes.
UDP and QUIC factories in the same project already use the correct
try { ... } catch { client.Dispose(); throw; }pattern; TCP and WebSocket diverged.Fix
Wrap connect-through-return in
try/catch+Dispose, mirroring UDP:src/NetConduit.Transport.Tcp/TcpMultiplexer.cs— bothCreateOptions(string host, int port)andCreateOptions(IPEndPoint endpoint)overloads.src/NetConduit.Transport.WebSocket/WebSocketMultiplexer.cs—CreateOptions(Uri, ...)(thestring urloverload delegates).src/NetConduit.Transport.WebSocket/WebSocketMuxListener.cs—CreateReconnectableClientOptions(Uri, ...).CreateServerOptionsin both transports is unaffected (server side does not construct outbound clients).Regression test
tests/NetConduit.Transport.Tcp.IntegrationTests/TcpMultiplexerFactoryLeakTests.cs— two tests, one perCreateOptionsoverload:Process.HandleCountafter warmup + GC.StreamFactory50 times against RFC 5737 TEST-NET-1 (192.0.2.1:1) with a 50ms per-attemptCancellationToken(deterministic across platforms —ConnectAsyncblocks on a non-routable address until cancelled).Process.HandleCountagain without intervening GC.< iterations/2.Pre-fix measurement (verified by
git stash-ing the production change and re-running): handle count grew by 55–57 over 50 iterations — almost exactly 1:1, confirming each leakedTcpClientretains one OS handle until finalization. Test fails loudly.Post-fix measurement: tests pass with negligible handle growth.
No equivalent WebSocket test included —
ClientWebSocketconnect leak observability is muddied by its internalHttpMessageInvokerhandle accounting, but the fix is structurally identical to the TCP fix and reviewed by diff.Verification
dotnet build -c Debug --nologo— 0 warnings, 0 errors.dotnet test -c Debug --nologo(full sweep) — 611 total, 610 succeeded, 1 pre-existing skip (MemoryLeak_SubMuxChaos_MemoryStaysBounded), 0 failed.