Re: RFR: 8277969: HttpClient SelectorManager shuts down when custom Executor rejects a task [v2]
On Wed, 9 Mar 2022 16:54:58 GMT, Daniel Fuchs wrote: >> These changes make sure that pending requests are terminated if the selector >> manager thread exits due to exceptions. >> This includes: >>1. completing CompletableFutures that were returned to the caller code >>2. cancelling requests that are in flight >>3. calling onError on BodySubscribers that may not have been completed >> Note that step 3 is necessary as certain CompletableFutures, such as those >> returned by `BodySubscribers.ofInputStream`, complete immediately, the >> operation being eventually completed when the last bite of the response is >> read. Completing a completable future that is already completed has no >> effect, this case is handled by completing the BodySubscriber too. > > Daniel Fuchs has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains three additional > commits since the last revision: > > - Incorporated review comments > - Merge branch 'master' into executor-shutdown-8277969 > - 8277969: HttpClient SelectorManager shuts down when custom Executor > rejects a task I went over the remaining changes in this PR and to me these changes look fine. - PR: https://git.openjdk.java.net/jdk/pull/7196
Re: RFR: 8277969: HttpClient SelectorManager shuts down when custom Executor rejects a task [v2]
On Thu, 10 Mar 2022 10:08:16 GMT, Jaikiran Pai wrote: >> tryRelease can be called multiple time - it will only decrement the ref >> counting once. It could happen when different threads notice that the >> operation is finished (usually due to some exceptions being propagated) and >> try concurrently to decrease the ref counter. It is very important to >> decrease the ref counter (otherwise the HttpClient may never shutdown when >> it's released) and equally very important not to decrease it twice for the >> same operation. Here we don't need to check whether it's been acquired >> because we know it's been acquired if we reach here. IIRC what we don't want >> to do is call tryRelease() if it's never been acquired. > >> Here we don't need to check whether it's been acquired because we know it's >> been acquired if we reach here. > > Hello Daniel, may be I am misreading the diff but from what I can see, the > `acquire()` method now returns a `boolean` which we are assigning to the > local `acquire`. But we aren't checking it for `true` for doing the > subsequent operation. So when it reaches the finally block here, as far as I > can see, it's not guaranteed that we did indeed `acquire` it. The acquire() method will return true the first time it's been called. And it is called only once. So we only need to check whether `acquired` is true at places where we are in doubt about whether the method has been called. The code that calls acquire() above could simply have ignored the result of `acquire()` and set the boolean to `true`. That said, it would not be wrong to check whether `acquired==true` here too - maybe I should do it for consistency... - PR: https://git.openjdk.java.net/jdk/pull/7196
Re: RFR: 8277969: HttpClient SelectorManager shuts down when custom Executor rejects a task [v2]
On Wed, 9 Mar 2022 12:09:48 GMT, Daniel Fuchs wrote:
>> src/java.net.http/share/classes/jdk/internal/net/http/HttpClientImpl.java
>> line 179:
>>
>>> 177: } catch (Throwable t) {
>>> 178: errorHandler.accept(command, t);
>>> 179: ASYNC_POOL.execute(command);
>>
>> Does this mean that we will now attempt to use an `ASYNC_POOL` even if a had
>> supplied an `Executor`? The `CompletableFuture#defaultExecutor` which is
>> what the `ASYNC_POOL` represents states:
>>
>> Returns the default Executor used for async methods that do not specify an
>> Executor.
>>
>> which would now contradict with this code. With the error handler logic in
>> place (one line above), do you think we should just give up on running the
>> `command` instead of passing to the default executor?
>>
>> Furthermore, if we do decide to use the default executor in cases like this,
>> is that something that all implementations of the `HttpClient` are expected
>> to do (i.e. would it be a specification) or is this more an implementation
>> detail.
>
> 1. Dependent actions added by the caller to the CF returned by the HttpClient
> are executed in the FJP
> 2. This is an exceptional case - we're doing this only when we're shutting
> down the HttpClient. I don't think we should document it - this is an
> implementation detail. But we should probably document that shutting down the
> executor while operations are still in progress can lead to
> unpredictable/unspecified behavior.
>
> Note: If we had VirtualThreads we would probably create a new VirtualThread
> here, rather than using the FJP.
Thank you for that clarification, especially this part which I wasn't aware of:
> 1. Dependent actions added by the caller to the CF returned by the HttpClient
> are executed in the FJP
-
PR: https://git.openjdk.java.net/jdk/pull/7196
Re: RFR: 8277969: HttpClient SelectorManager shuts down when custom Executor rejects a task [v2]
On Wed, 9 Mar 2022 11:55:27 GMT, Daniel Fuchs wrote: > Here we don't need to check whether it's been acquired because we know it's > been acquired if we reach here. Hello Daniel, may be I am misreading the diff but from what I can see, the `acquire()` method now returns a `boolean` which we are assigning to the local `acquire`. But we aren't checking it for `true` for doing the subsequent operation. So when it reaches the finally block here, as far as I can see, it's not guaranteed that we did indeed `acquire` it. - PR: https://git.openjdk.java.net/jdk/pull/7196
Re: RFR: 8277969: HttpClient SelectorManager shuts down when custom Executor rejects a task [v2]
> These changes make sure that pending requests are terminated if the selector > manager thread exits due to exceptions. > This includes: >1. completing CompletableFutures that were returned to the caller code >2. cancelling requests that are in flight >3. calling onError on BodySubscribers that may not have been completed > Note that step 3 is necessary as certain CompletableFutures, such as those > returned by `BodySubscribers.ofInputStream`, complete immediately, the > operation being eventually completed when the last bite of the response is > read. Completing a completable future that is already completed has no > effect, this case is handled by completing the BodySubscriber too. Daniel Fuchs has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains three additional commits since the last revision: - Incorporated review comments - Merge branch 'master' into executor-shutdown-8277969 - 8277969: HttpClient SelectorManager shuts down when custom Executor rejects a task - Changes: - all: https://git.openjdk.java.net/jdk/pull/7196/files - new: https://git.openjdk.java.net/jdk/pull/7196/files/9ad98bdf..6c6f3f1a Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7196=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7196=00-01 Stats: 66043 lines in 1879 files changed: 45834 ins; 10687 del; 9522 mod Patch: https://git.openjdk.java.net/jdk/pull/7196.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7196/head:pull/7196 PR: https://git.openjdk.java.net/jdk/pull/7196
