On Thu, 20 Feb 2025 18:44:53 GMT, Daniel Fuchs <dfu...@openjdk.org> wrote:
> Hi - Please find here a PR that improves streaming strategy in the HttpClient.
>
> The HttpClient currently waits until the full request body has been sent
> before starting to listen for a response. This is not optimal, in particular
> in cases where the server sends back e.g. 500 without even reading the body.
> It also prevents starting to stream the response body from the server before
> having sent the full request body, which prevents the server to stream back
> the request body to the client without reading it fully first.
>
> While writing a test to verify the fix, I also noticed a few places where
> additional tasks needed to be performed asynchronously (= delegated to the
> executor) to support this.
src/java.net.http/share/classes/jdk/internal/net/http/Exchange.java line 428:
> 426: var sendBody = exchImpl.sendBodyAsync();
> 427: CompletableFuture<Response> cf =
> exchImpl.getResponseAsync(executor);
> 428: sendBody.exceptionally((t) -> {
Why don't we mirror this as in `cf.exceptionally(t -> ...)`? That is, what if
receive-response-body fails while send-request-body is in flight? (Thinking of
push promises or alike early responses that server delivers without fully
consuming the request.)
src/java.net.http/share/classes/jdk/internal/net/http/Http1Exchange.java line
406:
> 404: .thenAccept(this::cancelIfFailed)
> 405: .thenAcceptAsync((s) -> requestMoreBody(),
> 406: exchange.executor().safeDelegate());
Shall we document the need to switch to this particular executor? (I can guess
it from the context of the PR, yet it is not apparent to me in isolation.)
src/java.net.http/share/classes/jdk/internal/net/http/Http1Exchange.java line
703:
> 701: bodySentCF.completeAsync(() -> this, exec);
> 702: } else {
> 703: exec.ensureExecutedAsync(this::requestMoreBody);
This means that _"the immediate run shortcut for non-selector threads"_
facilitated by `DelegatingExecutor::execute` will be skipped. Why?
src/java.net.http/share/classes/jdk/internal/net/http/Stream.java line 1113:
> 1111: // decremented on the HttpClient.
> 1112: cancelImpl(ex);
> 1113: }
Curious: This _fix_ is not specific to this PR, that is, this should have been
fixed even in the absence of the feature this PR is delivering, right?
src/java.net.http/share/classes/jdk/internal/net/http/Stream.java line 1124:
> 1122: if (debug.on())
> 1123: debug.log("RequestSubscriber: onSubscribe, request 1");
> 1124:
> exchange.executor().safeDelegate().execute(this::tryRequestMore);
I'd appreciate it if we can document the need to execute this asynchronously
using this particular executor – while it is not ultimate measure,
nevertheless, it is not apparent to me. 🙈
test/jdk/java/net/httpclient/PostFromGetTest.java line 31:
> 29: * @library /test/lib /test/jdk/java/net/httpclient/lib
> 30: * @build jdk.httpclient.test.lib.common.HttpServerAdapters
> jdk.test.lib.net.SimpleSSLContext
> 31: * ReferenceTracker PostFromGetTest
Is `PostFromGetTest` needed here?
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/23716#discussion_r1965316970
PR Review Comment: https://git.openjdk.org/jdk/pull/23716#discussion_r1965324035
PR Review Comment: https://git.openjdk.org/jdk/pull/23716#discussion_r1965327498
PR Review Comment: https://git.openjdk.org/jdk/pull/23716#discussion_r1965334124
PR Review Comment: https://git.openjdk.org/jdk/pull/23716#discussion_r1965332124
PR Review Comment: https://git.openjdk.org/jdk/pull/23716#discussion_r1965340519
