Re: RFR: 8343791: Socket.connect API should document whether the socket will be closed when hostname resolution fails or another error occurs [v2]
On Fri, 15 Nov 2024 19:14:00 GMT, Volkan Yazıcı 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 refreshed the contents of this pull request, and previous
> commits have been removed. The incremental views will show differences
> compared to the previous content of the PR. The pull request contains one new
> commit since the last revision:
>
> 8343791: Close `Socket` on `connect()` failures
I've added a lengthy comment to the JBS issue as there some decisions to make
on this issue.
-
PR Comment: https://git.openjdk.org/jdk/pull/22160#issuecomment-2480522300
Re: RFR: 8343791: Socket.connect API should document whether the socket will be closed when hostname resolution fails or another error occurs [v2]
> 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 refreshed the contents of this pull request, and previous
commits have been removed. The incremental views will show differences compared
to the previous content of the PR. The pull request contains one new commit
since the last revision:
8343791: Close `Socket` on `connect()` failures
-
Changes:
- all: https://git.openjdk.org/jdk/pull/22160/files
- new: https://git.openjdk.org/jdk/pull/22160/files/512e1fed..8a36a881
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=22160&range=01
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=22160&range=00-01
Stats: 0 lines in 0 files changed: 0 ins; 0 del; 0 mod
Patch: https://git.openjdk.org/jdk/pull/22160.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/22160/head:pull/22160
PR: https://git.openjdk.org/jdk/pull/22160
