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

[Alan Bateman]

On Wed, 20 Nov 2024 14:48:56 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 with a new target base due to a merge or a rebase. The pull request now contains eight commits:
> 
>  - Merge master
>  - Catch all `bind()` and `connect()` exceptions in ctor
>  - Use safe-`close()` before throwing `UHE` in `connect()`
>  - Update docs for `connect()` not closing on `UHE`
>  - Incorporate more review feedback
>  - Apply review feedback from Alan
>  - Apply review suggestions by Alan & Daniel
>  - 8343791: Close `Socket` on `connect()` failures

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

> 571:      *
> 572:      * <p> If the connection cannot be established, then the socket is closed
> 573:      * and an {@link IOException} is thrown.

What happened to "the endpoint is an unresolved InetSocketAddress"? If part of the change is to close when called with an unresolved address then it needs to be part of the specification.

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

> 659: 
> 660:         if (epoint.isUnresolved()) {
> 661:             UnknownHostException exception = new UnknownHostException(epoint.getHostName() + " is unresolved");

In passing, if you replace this with `var uhe = new UnknownHostException(...)` then the later "throw uhe` will be clearer than `throw exception`.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/22160#discussion_r1850471583
PR Review Comment: https://git.openjdk.org/jdk/pull/22160#discussion_r1850473731