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

[Alan Bateman]

On Thu, 21 Nov 2024 08:32:55 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 four additional commits since the last revision:
> 
>  - Revert `UHE` message change in `NioSocketImpl`
>  - Remove self-reference guard in `closeSuppressingExceptions()`
>  - Add back incorrectly removed `SocketTimeoutException` Javadoc
>  - Remove unused `port` variable

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

> 610:             close();
> 611:             if (ioe instanceof InterruptedIOException) {
> 612:                 throw new SocketException("Closed by interrupt");

SocketTimeoutException is InterruptedIOException so need to distinguish this from the case where a virtual thread is interrupt during connect. The change for NioSocketImpl needs to be like this (once exception handle in switch comes then we can improve this).


             }
         } catch (IOException ioe) {
             close();
-            if (ioe instanceof InterruptedIOException) {
+            if (ioe instanceof SocketTimeoutException) {
                 throw ioe;
+            } else if (ioe instanceof InterruptedIOException) {
+                assert Thread.currentThread().isVirtual();
+                throw new SocketException("Closed by interrupt");
             } else {
                 throw SocketExceptions.of(ioe, isa);
             }

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

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