On Tue, 19 Nov 2024 20:18:41 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 feedback from Alan
src/java.base/share/classes/java/net/Socket.java line 503:
> 501: // `connect()` already closes the socket on failures, yet
> `bind()` doesn't, and hence:
> 502: closeQuietly(throwable);
> 503: throw throwable;
Would you mind adding `assert address instanceof InetSocketAddress isa &&
!isa.isUnresolved();` to the start of this (private) constructor as that will
ensure we have the right pre-conditions and avoid further discussion about
other socket address types.
src/java.base/share/classes/java/net/Socket.java line 625:
> 623: * is already connected or the socket is closed
> 624: * @throws UnknownHostException if the endpoint is an unresolved
> 625: * {@link InetSocketAddress}, the socket is closed
It might be better to drop ", the socket is closed" from the exception
description as it may be mis-read to mean that it gets thrown when the socket
is closed.
src/java.base/share/classes/java/net/Socket.java line 662:
> 660: * </ol>
> 661: * <p> Besides above noted cases, the socket will also be closed
> when a
> 662: * connection attempt fails with an {@link IOException}.
Is this a left-over, I assume it should be removed.
src/java.base/share/classes/java/net/Socket.java line 669:
> 667: * is already connected or the socket is closed
> 668: * @throws SocketTimeoutException if timeout expires before
> connecting,
> 669: * the socket is closed
The updated method description makes it clear that the the socket will be
closed if the timeout expires before the connection is established, shouldn't
need to change the exception description.
src/java.base/share/classes/java/net/Socket.java line 709:
> 707: }
> 708:
> 709: try {
At (old) L680, it checks that endpoint is an InetSocketAddress. We should also
check that it resolved, as in:
if (epoint.isUnresolved()) {
close();
throw new UnknownHostException(epoint.getHostName() + " is
unresolved");
}
The benefit is the the constructor does the validation before is delegates to
the SocketImpl. We could change the SocketImpl to throw IAE for this case, it
doesn't matter because it will never happen if Socket rejects it.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/22160#discussion_r1849835460
PR Review Comment: https://git.openjdk.org/jdk/pull/22160#discussion_r1849827508
PR Review Comment: https://git.openjdk.org/jdk/pull/22160#discussion_r1849827807
PR Review Comment: https://git.openjdk.org/jdk/pull/22160#discussion_r1849829296
PR Review Comment: https://git.openjdk.org/jdk/pull/22160#discussion_r1849844196
