On Wed, 20 Nov 2024 15:24:54 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 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
