Copilot commented on code in PR #2645:
URL: https://github.com/apache/plc4x/pull/2645#discussion_r3629938095
##########
plc4j/transports/tcp/src/test/java/org/apache/plc4x/java/transport/tcp/TcpTransportInstanceTest.java:
##########
@@ -290,6 +290,49 @@ void testConcurrentReadWrite() throws Exception {
assertTrue(doneLatch.await(10, TimeUnit.SECONDS));
}*/
+ @Test
+ void testConstructor_failedBind_doesNotLeakFileDescriptors() throws
Exception {
+ // getOpenFileDescriptorCount() is only exposed on Unix-flavoured JVMs
+ com.sun.management.UnixOperatingSystemMXBean osBean;
+ try {
+ osBean = (com.sun.management.UnixOperatingSystemMXBean)
+
java.lang.management.ManagementFactory.getOperatingSystemMXBean();
+ } catch (ClassCastException e) {
+ org.junit.jupiter.api.Assumptions.assumeTrue(false,
+ "Open file descriptor count not available on this platform");
+ return;
+ }
Review Comment:
`Assumptions.assumeTrue(false, ...)` plus a `return` is a bit awkward and
can be made clearer/idiomatic. Prefer an `instanceof` check and a single
assumption (no `try/catch` needed), e.g. obtain the bean as
`OperatingSystemMXBean`, check `instanceof UnixOperatingSystemMXBean`, and
`assumeTrue(...)` before casting. This also avoids relying on
`ClassCastException` for control flow and makes it obvious the test is skipped
on unsupported platforms.
##########
plc4j/transports/tcp/src/test/java/org/apache/plc4x/java/transport/tcp/TcpTransportInstanceTest.java:
##########
@@ -290,6 +290,49 @@ void testConcurrentReadWrite() throws Exception {
assertTrue(doneLatch.await(10, TimeUnit.SECONDS));
}*/
+ @Test
+ void testConstructor_failedBind_doesNotLeakFileDescriptors() throws
Exception {
+ // getOpenFileDescriptorCount() is only exposed on Unix-flavoured JVMs
+ com.sun.management.UnixOperatingSystemMXBean osBean;
+ try {
+ osBean = (com.sun.management.UnixOperatingSystemMXBean)
+
java.lang.management.ManagementFactory.getOperatingSystemMXBean();
+ } catch (ClassCastException e) {
+ org.junit.jupiter.api.Assumptions.assumeTrue(false,
+ "Open file descriptor count not available on this platform");
+ return;
+ }
+
+ // Occupy a specific local port so binding our own channel to it
deterministically fails
+ // with BindException
+ try (ServerSocketChannel occupier = ServerSocketChannel.open()) {
+ occupier.bind(new InetSocketAddress("127.0.0.1", 0));
+ int occupiedPort = ((InetSocketAddress)
occupier.getLocalAddress()).getPort();
+
+ TcpTransportConfiguration config = new TcpTransportConfiguration();
+ config.receiveBufferSize = 81920;
+ config.localAddress = "127.0.0.1";
+ config.localPort = occupiedPort;
+ // Never actually reached - bind() fails first - but must be a
well-formed address.
+ InetSocketAddress remoteAddress = new
InetSocketAddress("127.0.0.1", occupiedPort);
+
+ int attempts = 20;
+ long before = osBean.getOpenFileDescriptorCount();
+ for (int i = 0; i < attempts; i++) {
+ assertThrows(TransportException.class, () ->
+ new TcpTransportInstance(remoteAddress, config,
AuditLog.builder().build())
+ );
+ }
+ long grown = osBean.getOpenFileDescriptorCount() - before;
+
+ // 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 <= 1,
+ "Open file descriptor count changed by " + grown + " across "
+ attempts
+ + " failed bind attempts; expected no per-attempt growth
(possible SocketChannel leak)");
Review Comment:
This assertion can be flaky in CI because process-level FD count can
legitimately change due to unrelated JVM activity (classloading/JFR, other
concurrent tests, etc.), and (in the *unfixed* version) GC/cleaners might close
some leaked channels mid-loop, reducing observed growth. To make the test more
robust, consider (mandatory for stability) loosening the threshold and/or
detecting “linear per-attempt growth” instead of an absolute before/after delta
(e.g., sample FD count after each iteration and assert it does not increase
consistently with attempts). This will preserve leak detection while reducing
sensitivity to unrelated FD churn.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
