On Thu, 21 Nov 2024 08:32:55 GMT, Volkan Yazıcı <d...@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
