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

[Alan Bateman]

On Fri, 29 Nov 2024 09:11:08 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 three additional commits since the last revision:
> 
>  - Use more fine-grained suppression
>  - Further simplify tests
>  - Extend `translateToSocketException()` with `AlreadyConnectedException`

src/java.base/share/classes/sun/nio/ch/Net.java line 171:

> 169:             nx = newSocketException("Already bound");
> 170:         else if (x instanceof AlreadyConnectedException)
> 171:             nx = newSocketException("already connected");

Probably better to use "Already connected" here, that will mean it has the same case as the exception message in SocketAdaptor.connect.

test/jdk/java/net/Socket/ConnectFailTest.java line 72:

> 70: 
> 71:     /**
> 72:      * Verifies socket is closed when {@code unboundSocket.connect()} fails.

Did you mean to say "unboundSocket.connect"? I assume to mean to that it tests that an unbound Socket is closed when connect fails.

test/jdk/java/net/Socket/ConnectFailTest.java line 189:

> 187: 
> 188:     @SuppressWarnings("ResultOfMethodCallIgnored")
> 189:     private static void continuouslyAcceptConnections(ServerSocket serverSocket) {

I realize the test has gone through several rounds of simplification. I think it can be simplified further as there is no need to use an ExecutorService or have another thread managing a listen socket and accepting connections. The reason is that all the supported operating systems queue accepted connections. This means it can be all done synchronously, e.g. testConnectedSocket and testConnectedSocketWithUnresolvedAddress can setup the loopback connection with code like this:

        try (socket) {
            withListener(listener -> {
                socket.connect(listener.getLocalSocketAddress());
                try (var peer = listener.accept()) {
                    :
                }
            });
        }

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

PR Review Comment: https://git.openjdk.org/jdk/pull/22160#discussion_r1863395100
PR Review Comment: https://git.openjdk.org/jdk/pull/22160#discussion_r1863374404
PR Review Comment: https://git.openjdk.org/jdk/pull/22160#discussion_r1863394130