On Wed, 9 Mar 2022 10:04:57 GMT, Jaikiran Pai <j...@openjdk.org> 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.
>
> src/java.net.http/share/classes/jdk/internal/net/http/Http1AsyncReceiver.java
> line 501:
>
>> 499: debug.log("recorded " + t + "\n\t delegate: " + delegate
>> 500: + "\n\t queue.isEmpty: " + queue.isEmpty()
>> 501: + " stopRequested: " + stopRequested, ex);
>
> Should this also be preceded with a `\n\t ` to be consistent with the rest of
> this log message?
Good point - will do
> src/java.net.http/share/classes/jdk/internal/net/http/Http1Response.java line
> 366:
>
>> 364: refCountTracker.tryRelease();
>> 365: }
>> 366: });
>
> I see that later in this code there's another `finally` block where we have
> now added a `tryRelease` call which first checks the `acquired` flag. A
> couple of questions:
> 1. Should this above line too first check the `acquire` flag?
> 2. Should we reset the `acquire` local variable once we `tryRelease` here so
> as to prevent another `tryRelease` in the other `finally` block? A short
> glance at the implementation of `tryRelease` suggests that multiple
> `tryRelease()` with a single `acquire()` won't cause a problem in its state
> but it does end up logging some messages, which perhaps could be avoided.
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.
> src/java.net.http/share/classes/jdk/internal/net/http/Http1Response.java line
> 374:
>
>> 372: cf.completeExceptionally(t);
>> 373: } finally {
>> 374: if (acquired) refCountTracker.tryRelease();
>
> I see that before this change, we never did a `tryRelease` in this finally
> block. Was that a separate bug in itself and something that is being fixed as
> part of this PR?
I've been fighting with trying to get the ref counter back to 0 after
Executor::shutdown has been called.
Throwing TaskRejectedException (especially when
`SequentialScheduler::runOrSchedule(Executor)` is called) is very disruptive to
the logic.
In practice - it does not matter as much provided that all resources have been
released and all body subscribers have been completed - and that's what this PR
does.
> src/java.net.http/share/classes/jdk/internal/net/http/Http1Response.java line
> 647:
>
>> 645: if (t instanceof EOFException && parser != null &&
>> 646: parser instanceof UnknownLengthBodyParser) {
>> 647: ((UnknownLengthBodyParser)parser).complete();
>
> A minor note - while we are it, perhaps the new `instanceof` feature can be
> used here to avoid this cast?
Good point.
> src/java.net.http/share/classes/jdk/internal/net/http/Http1Response.java line
> 728:
>
>> 726: } else {
>> 727: if (debug.on()) {
>> 728: debug.log("no parser");
>
> Should we perhaps log the error message (`Throwable::getMessage`) to provide
> some kind of hints in the logs when this message is logged? Perhaps `Ignoring
> error since no parser available: + error.getMessage()`?
Good idea
> 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.
> src/java.net.http/share/classes/jdk/internal/net/http/HttpClientImpl.java
> line 199:
>
>> 197: // the selector manager thread exists abnormally.
>> 198: // The request, its id, and its completable future, are stored in a
>> record-like
>> 199: // PendingRequest object added to the pending requests set
>> (pendingRequests).
>
> I'm not too familiar with the HttpClient code yet. Before this change, was
> there ever a place where we would end up holding onto the request instances
> in memory and prevent them from being garbage collected? Would this change
> cause unintentional resource constraints in a regular/good scenario (where
> the `SelectorManager` thread hasn't exited abnormally and is still alive) or
> in cases where HTTP responses are slow to come back from servers?
In the normal course of action the HttpClient will not shutdown if any
operation are still in progress. So holding onto the multiexchange here doesn't
prevent the client from shutting down, since the multiexchange will be remove
from this list when the operation completes.
> src/java.net.http/share/classes/jdk/internal/net/http/HttpClientImpl.java
> line 302:
>
>> 300: var msg = reason instanceof RejectedExecutionException
>> 301: ? reason.getClass() : reason;
>> 302: client.debug.log("aborting pending requests due to: %s",
>> msg);
>
> Any reason why we are using a different log message for
> `RejectedExecutionException`? Would it instead be useful to let the
> underlying `message` from the `RejectedExecutionException` be logged too just
> like we are doing for other exception types here?
IIRC the message was too long and not very informative.
> src/java.net.http/share/classes/jdk/internal/net/http/HttpClientImpl.java
> line 308:
>
>> 306: while (!pendingRequests.isEmpty()) {
>> 307: var pendings = pendingRequests.iterator();
>> 308: while (pendings.hasNext()) {
>
> Would there be a case where while we are aborting these current pending
> requests, some user code does a `sendAsync` and we end up add a new pending
> request to the `Set`? Would we then end up holding those new requests in the
> `Set` never to be cleared again because, I guess, `abortPendingRequests`
> won't be called again?
I think that's been taken care of. The first thing we do is set up a flag that
we're shutting down. I remember asking myself the same question and making sure
it doesn't happen - that is - everything will be aborted.
-------------
PR: https://git.openjdk.java.net/jdk/pull/7196
