Return the TLS write buffer when TCP client connection socket setup fails - #4941
Return the TLS write buffer when TCP client connection socket setup fails#4941pepone wants to merge 1 commit into
Conversation
pepone
added
ai-audit
There was a problem hiding this comment.
🟢 Approval recommended
The reviewed changes address both failure paths and include regression coverage.
Pull request overview
This pull request fixes resource leaks during failed TCP/TLS connection construction.
Changes:
- Defers TLS write-buffer rental until socket setup succeeds.
- Disposes sockets when buffer rental fails.
- Adds regression tests for client and server failure paths.
File summaries
| File | Summary |
|---|---|
tests/IceRpc.Tests/Transports/Tcp/TcpTransportTests.cs |
Adds resource-cleanup regression tests. |
src/IceRpc/Transports/Tcp/Internal/TcpConnection.cs |
Refactors socket ownership and construction cleanup. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
bernardnormier
left a comment
bernardnormier
left a comment
There was a problem hiding this comment.
Let's keep the constructors as they are. For the client leak, calling Dispose() instead of Socket.Dispose() in
the two catch blocks of the TcpClientConnection constructor returns the buffer as well: the SSL stream is still
null and closing an unconnected socket is fine. Two lines, no CreateSocket, no base constructor change.
The server path needs a pool whose Rent throws, which the default pool never does for our segment sizes. Drop that
part.
No test for this one: the leak is only observable through a pool built to count rentals, and the fix is two lines.
No What's Changed entry either: None — the default pool doesn't leak, and a client socket setup failure means every connection attempt fails.
…ails When the TcpClientConnection constructor failed to bind or configure its socket, its catch blocks disposed the socket but never returned the TLS write buffer rented in the base-constructor initializer. The catch blocks now call Dispose() instead of Socket.Dispose(): the SSL stream is still null and the socket is not connected, so Dispose() closes the socket and returns the write buffer to the pool. Fixes icerpc#4828
58cc1da to
e391a32
Compare
pepone
changed the title
|
🤖 Claude: Reworked as suggested in e391a32: the constructors are back as they were, the two catch blocks in the |
There was a problem hiding this comment.
🟡 Changes recommended
It lacks regression coverage and does not address all resource-loss paths identified by #4828.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 2
- Review effort level: Balanced
| catch (SocketException exception) | ||
| { | ||
| Socket.Dispose(); | ||
| Dispose(); |
| catch | ||
| { | ||
| Socket.Dispose(); | ||
| Dispose(); |
When the
TcpClientConnectionconstructor failed to bind or configure its socket (for example aLocalNetworkAddressthe socket cannot bind), its two catch blocks disposed the socket but never returned the TLS write buffer rented in the
base-constructor initializer. Since the constructor throws, no instance escapes and
Dispose(), which returns thebuffer, was never called.
The catch blocks now call
Dispose()instead ofSocket.Dispose(). At that point the SSL stream is still null and thesocket is not connected, so
Dispose()closes the socket and returns the write buffer to the pool.Fixes #4828
What's Changed entry
None — the default pool doesn't leak, and a client socket setup failure means every connection attempt fails.