RFR: 8343791: Socket.connect API should document whether the socket will be closed when hostname resolution fails or another error occurs [v22]

[Alan Bateman]

On Thu, 28 Nov 2024 09:12:58 GMT, Volkan Yazıcı <duke at 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