On Tue, 19 Nov 2024 08:56:18 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 one additional
> commit since the last revision:
>
> Apply review suggestions by Alan & Daniel
src/java.base/share/classes/java/net/Socket.java line 500:
> 498: bind(localAddr);
> 499: connect(address);
> 500: } catch (IOException | IllegalArgumentException |
> SecurityException error) {
Naming the exception "error" is a bit strange as this isn't catching Errors. I
think the only change need to the Socket constructor is to change line 500 to
catch Throwable, leave the try-catch to add the suppressed exception as it's
much more readable.
src/java.base/share/classes/java/net/Socket.java line 617:
> 615: * </ol>
> 616: * <p> Besides above noted cases, the socket will also be closed
> when a
> 617: * connection attempt fails with an {@link IOException}.
I think it would be better to put the new paragraph before the paragraph on
interrupt. I think we'll end up with something like " If the endpoint is an
unresolved InetSocketAddress, the connection cannot be established, or the
timeout expires before the connection is established, then the Socket is closed
and IOException is thrown".
src/java.base/share/classes/java/net/Socket.java line 623:
> 621: * is already connected or the socket is closed
> 622: * @throws UnknownHostException if the endpoint address is
> 623: * {@linkplain InetSocketAddress#isUnresolved() not
> resolved},
I think better to say "if the endpoint is an unresolved InetSocketAddress".
src/java.base/share/classes/java/net/Socket.java line 706:
> 704: try {
> 705: getImpl().connect(epoint, timeout);
> 706: } catch (IOException error) {
At L680 we reject an endpoint that is not an InetSocketAddresss. I think we
should follow this with a check to ensure that the address is resolved, that
way Socket does all the validation before delegation to the SocketImpl. It
means if getImpl().connect(..) throws any exception or error then you can close
the Socket, meaning you can catch Throwable.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/22160#discussion_r1848031079
PR Review Comment: https://git.openjdk.org/jdk/pull/22160#discussion_r1848054889
PR Review Comment: https://git.openjdk.org/jdk/pull/22160#discussion_r1848044455
PR Review Comment: https://git.openjdk.org/jdk/pull/22160#discussion_r1848038335
