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

[Alan Bateman]

On Tue, 19 Nov 2024 08:56:18 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 suggestions by Alan & Daniel

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

> 498:                 bind(localAddr);
> 499:             connect(address);
> 500:         } catch (IOException | IllegalArgumentException | SecurityException error) {

Naming the exception "error" is a bit strange as this isn't catching Errors. I think the only change need to the Socket constructor is to change line 500 to catch Throwable, leave the try-catch to add the suppressed exception as it's much more readable.

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

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

I think it would be better to put the new paragraph before the paragraph on interrupt. I think we'll end up with something like " If the endpoint is an unresolved InetSocketAddress, the connection cannot be established, or the timeout expires before the connection is established, then the Socket is closed and IOException is thrown".

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

> 621:      *          is already connected or the socket is closed
> 622:      * @throws  UnknownHostException if the endpoint address is
> 623:      *          {@linkplain InetSocketAddress#isUnresolved() not resolved},

I think better to say "if the endpoint is an unresolved InetSocketAddress".

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

> 704:         try {
> 705:             getImpl().connect(epoint, timeout);
> 706:         } catch (IOException error) {

At L680 we reject an endpoint that is not an InetSocketAddresss. I think we should follow this with a check to ensure that the address is resolved, that way Socket does all the validation before delegation to the SocketImpl. It means if getImpl().connect(..) throws any exception or error then you can close the Socket, meaning you can catch Throwable.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/22160#discussion_r1848031079
PR Review Comment: https://git.openjdk.org/jdk/pull/22160#discussion_r1848054889
PR Review Comment: https://git.openjdk.org/jdk/pull/22160#discussion_r1848044455
PR Review Comment: https://git.openjdk.org/jdk/pull/22160#discussion_r1848038335