fix(plc4j/transports-tcp): close SocketChannel on failed bind/socket-option setup#2645
Open
mkribs wants to merge 1 commit into
Open
fix(plc4j/transports-tcp): close SocketChannel on failed bind/socket-option setup#2645mkribs wants to merge 1 commit into
mkribs wants to merge 1 commit into
Conversation
…option setup TcpTransportInstance's constructor only assigned the opened SocketChannel to the socketChannel field after bind(), socket options and connect() all succeeded. If bind() or a socket-option call threw (e.g. a pinned local port still held by a previous failed attempt), the already-open channel was never closed - each such failure leaked one fd. connect() failures (timeout/refused) are unaffected since Socket.connect() already closes itself internally on failure.
2 tasks
There was a problem hiding this comment.
Pull request overview
This PR fixes a file-descriptor leak in TcpTransportInstance when socket setup fails before connect() (e.g., bind() or socket-option configuration throwing). It ensures an opened SocketChannel is closed on constructor failure, and adds a regression test to detect descriptor growth across repeated failed binds.
Changes:
- Refactor
TcpTransportInstanceconstructor to keep theSocketChannelin a local variable until all setup steps succeed, and explicitly close it onIOException. - Add a regression test that forces a deterministic
BindExceptionscenario and checks that open file descriptor counts do not grow across repeated failures.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| plc4j/transports/tcp/src/main/java/org/apache/plc4x/java/transport/tcp/TcpTransportInstance.java | Ensures the SocketChannel is closed if bind/socket-option/connect setup throws before construction completes. |
| plc4j/transports/tcp/src/test/java/org/apache/plc4x/java/transport/tcp/TcpTransportInstanceTest.java | Adds a regression test to detect fd leaks across repeated failed constructor attempts on Unix-like JVMs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+328
to
+333
| // Without the fix each failed constructor call leaks the SocketChannel it opened | ||
| // before bind() failed, so fd count grows by ~1 per attempt. With the fix it stays flat. | ||
| assertTrue(grown < attempts, | ||
| "Open file descriptor count grew by " + grown + " across " + attempts | ||
| + " failed bind attempts - each failed TcpTransportInstance constructor call " | ||
| + "is leaking its SocketChannel instead of closing it"); |
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.
TcpTransportInstance's constructor only assigned the openedSocketChannelto its field afterbind(), socket options, andconnect()all succeeded. Ifbind()or a socket-option call threw (e.g. a pinned local port still held by a previous failed attempt), the already-open channel was never closed - leaking one fd per failed attempt.Note:
connect()failures (timeout/refused) are not affected -Socket.connect(SocketAddress, int)already closes itself internally on failure (confirmed empirically). Only failures beforeconnect()is reached (bind, socket options) leak.Fix: hold the channel in a local variable, only promote it to the
socketChannelfield once every setup step succeeds; explicitly close it in the constructor's catch block otherwise.Testing: added
testConstructor_failedBind_doesNotLeakFileDescriptors, which deterministically forces aBindException(occupies a local port, configures the transport to bind to that same port) and asserts the open-file-descriptor count stays flat across repeated failures viaUnixOperatingSystemMXBean. Verified the test fails without this fix (fd count grows ~1 per attempt) and passes with it, on Linux.closes #2644