On Thu, 28 Nov 2024 09:12:58 GMT, Volkan Yazıcı <d...@openjdk.org> wrote:
>> This PR, addressing 8343791, changes `Socket::connect()` methods to close >> the `Socket` in the event that the connection cannot be established, the >> timeout expires before the connection is established, or the socket address >> is unresolved. >> >> `tier3` tests pass against the 9f00f61d3b7fa42a5e23a04f80bb4bb1a2076ef2. > > Volkan Yazıcı has updated the pull request incrementally with two additional > commits since the last revision: > > - Don't exceed 120 characters per line > - Rework functional interfaces in tests test/jdk/java/net/Socket/ConnectFailTest.java line 57: > 55: * @test > 56: * @bug 8343791 > 57: * @summary verifies the socket state after `connect()` failures I think you can beef up the @summary a bit because the test checks that Socket.connect throws the expected exception and completes with the Socket in the expected state. test/jdk/java/net/Socket/ConnectFailTest.java line 59: > 57: * @summary verifies the socket state after `connect()` failures > 58: * @library /test/lib > 59: * @run junit/othervm --add-opens java.base/java.net=ALL-UNNAMED > ConnectFailTest I assume the --add-opens is left over from a few iteration. For future reference, the @modules tag will do this for you, do need to specify it to the @run tag. test/jdk/java/net/Socket/ConnectFailTest.java line 105: > 103: ConnectFailTest::createConnectedNioSocket, > 104: executable -> > assertThrows(AlreadyConnectedException.class, executable)); > 105: } I assume that testConnectedSocket and testConnectedNioSocket could be @ParameterizedTest so it's called with a new created Socket. test/jdk/java/net/Socket/ConnectFailTest.java line 139: > 137: assertFalse(socket.isBound()); > 138: assertFalse(socket.isConnected()); > 139: assertThrows(IOException.class, () -> > socket.connect(UNRESOLVED_ADDRESS)); The specified exception for this case is UHE so the assertThrows can be sharper. test/jdk/java/net/Socket/ConnectFailTest.java line 234: > 232: // Accept connections in the background to avoid blocking > the caller > 233: executorService.submit(() -> > acceptConnections(serverSocket)); > 234: serverSocketConsumer.accept(serverSocket); I think it just adds complexity to use a virtual thread here. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/22160#discussion_r1862284903 PR Review Comment: https://git.openjdk.org/jdk/pull/22160#discussion_r1862283789 PR Review Comment: https://git.openjdk.org/jdk/pull/22160#discussion_r1862287614 PR Review Comment: https://git.openjdk.org/jdk/pull/22160#discussion_r1862294280 PR Review Comment: https://git.openjdk.org/jdk/pull/22160#discussion_r1862301524
