On Mon, 24 Jan 2022 14:25:09 GMT, Daniel Fuchs <dfu...@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.
Hello Daniel, I couldn't fully finish the review, but added my initial
comments/questions.
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?
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?
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.
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.
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?
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?
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?
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?
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()`?
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()`?
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.
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?
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?
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?
Perhaps I shouldn't worry about it, since maybe in a regular case, the requests
that we are caching here will all be alive (from a GC point of view) even if
this caching isn't involved?
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?
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?
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?
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?
-------------
PR: https://git.openjdk.java.net/jdk/pull/7196
