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

[Alan Bateman]

On Tue, 19 Nov 2024 20:18:41 GMT, Volkan Yazıcı <duke at openjdk.org> wrote:

>> This PR, addressing 8343791, enhances `Socket#connect()` and effectively makes it close the `Socket` if `SocketImpl#connect()` fails with a
>> 
>> 1. `SocketTimeoutException`
>> 2. `InterruptedIOException` in an interrupted vthread
>> 3. `IOException` that is *not* an `UnknownHostException`
>> 
>> On the other hand, socket will *not* be closed if `SocketImpl#connect()` fails with a
>> 
>> * `InterruptedIOException` that is neither a `SocketTimeoutException`, nor from an interrupted vthread
>> * `UnknownHostException`
>> 
>> Note that in case of an `UHE`, `Socket` and `SocketImpl` states will match, i.e., `SOCKET_CREATED`.
>> 
>> One peculiar detail retained from the old code is that `InterruptedIOException` is causing a socket close *iff* we are in an interrupted vhtread. (Guess: To ensure resources within a vthread task scope is released in tandem with the completion of the scope?) I don't know why other `InterruptedIOException` cases are left out.
>> 
>> Before "8284161: Implementation of Virtual Threads (Preview)" (9583e365), `connect()` failures were getting propagated untouched. 8284161 added the logic to throw `SocketException("Closed by interrupt")` *and* close the socket if `InterruptedIOException` is caught in an interrupted vthread.
>> 
>> Judging from the last state of the `Socket#connect()` javadoc, I find it very difficult to wrap ones mind around in which cases the socket will be closed. As a user, I'd be in favor of a simpler approach – e.g., `try { socketImpl.connect(); } catch (Exception e) { closeQuietly(e); throw e }` – at the cost of incorporating a backward incompatible behavioral change. I would appreciate your advice on how shall we proceed further.
>
> Volkan Yazıcı has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Apply review feedback from Alan

src/java.base/share/classes/java/net/Socket.java line 503:

> 501:             // `connect()` already closes the socket on failures, yet `bind()` doesn't, and hence:
> 502:             closeQuietly(throwable);
> 503:             throw throwable;

Would you mind adding `assert address instanceof InetSocketAddress isa && !isa.isUnresolved();` to the start of this (private) constructor as that will ensure we have the right pre-conditions and avoid further discussion about other socket address types.

src/java.base/share/classes/java/net/Socket.java line 625:

> 623:      *          is already connected or the socket is closed
> 624:      * @throws  UnknownHostException if the endpoint is an unresolved
> 625:      *          {@link InetSocketAddress}, the socket is closed

It might be better to drop ", the socket is closed" from the exception description as it may be mis-read to mean that it gets thrown when the socket is closed.

src/java.base/share/classes/java/net/Socket.java line 662:

> 660:      * </ol>
> 661:      * <p> Besides above noted cases, the socket will also be closed when a
> 662:      * connection attempt fails with an {@link IOException}.

Is this a left-over, I assume it should be removed.

src/java.base/share/classes/java/net/Socket.java line 669:

> 667:      *          is already connected or the socket is closed
> 668:      * @throws  SocketTimeoutException if timeout expires before connecting,
> 669:      *          the socket is closed

The updated method description makes it clear that the the socket will be closed if the timeout expires before the connection is established, shouldn't need to change the exception description.

src/java.base/share/classes/java/net/Socket.java line 709:

> 707:         }
> 708: 
> 709:         try {

At (old) L680, it checks that endpoint is an InetSocketAddress. We should also check that it resolved, as in:

        if (epoint.isUnresolved()) {
            close();
            throw new UnknownHostException(epoint.getHostName() + " is unresolved");
        }

The benefit is the the constructor does the validation before is delegates to the SocketImpl. We could change the SocketImpl to throw IAE for this case, it doesn't matter because it will never happen if Socket rejects it.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/22160#discussion_r1849835460
PR Review Comment: https://git.openjdk.org/jdk/pull/22160#discussion_r1849827508
PR Review Comment: https://git.openjdk.org/jdk/pull/22160#discussion_r1849827807
PR Review Comment: https://git.openjdk.org/jdk/pull/22160#discussion_r1849829296
PR Review Comment: https://git.openjdk.org/jdk/pull/22160#discussion_r1849844196