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

[Alan Bateman]

On Wed, 20 Nov 2024 15:24:54 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 two additional commits since the last revision:
> 
>  - Replace `UnknownHostException exception` with `var uhe`
>  - Fix `connect()` Javadoc on `UHE` results in socket close

The API + implementation changes have gone through many iterations but I think we've got to a good place now. It should be possible to create or update the CSR.

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

> 635:      *          is already connected or the socket is closed
> 636:      * @throws  UnknownHostException if the endpoint is an unresolved
> 637:      *          {@link InetSocketAddress}

It looks like you deleted the throws SocketTimeoutException by mistake.

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

> 1605:         } catch (IOException exception) {
> 1606:             if (exception != parentException) {
> 1607:                 parentException.addSuppressed(exception);

I don't think you need the identity check, close would be very broken if it threw the enclosing exception.

src/java.base/share/classes/sun/nio/ch/NioSocketImpl.java line 567:

> 565:         InetSocketAddress isa = (InetSocketAddress) remote;
> 566:         if (isa.isUnresolved()) {
> 567:             throw new UnknownHostException("Unresolved address: " + isa.getHostName());

The changes means this change isn't needed now. I'm really tempted to say we change the unsupported address type to throw UOE, and the unresolved case to throw IAE, but don't have the complicated this PR any more, plus the SocketImpl API is hugely under specified so might result in more discussion that is worth it.

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

PR Comment: https://git.openjdk.org/jdk/pull/22160#issuecomment-2489263933
PR Review Comment: https://git.openjdk.org/jdk/pull/22160#discussion_r1850779189
PR Review Comment: https://git.openjdk.org/jdk/pull/22160#discussion_r1850781200
PR Review Comment: https://git.openjdk.org/jdk/pull/22160#discussion_r1850782665