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

[Volkan Yazıcı]

On Thu, 28 Nov 2024 14:22:23 GMT, Alan Bateman <alanb at openjdk.org> wrote:

>> 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.

Fixed.

> 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.

You beat me to it. I was just simplifying this to `junit ConnectFailTest` and saw your review. Fixed.

> 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.

Fixed.

> 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.

Replaced with `newSingleThreadExecutor()`.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/22160#discussion_r1863183980
PR Review Comment: https://git.openjdk.org/jdk/pull/22160#discussion_r1863183618
PR Review Comment: https://git.openjdk.org/jdk/pull/22160#discussion_r1863189514
PR Review Comment: https://git.openjdk.org/jdk/pull/22160#discussion_r1863190367