RFR: 8293786: HttpClient will not send more than 64 kb of data from the 2nd request in http2

[Daniel Fuchs]

On Wed, 22 Feb 2023 11:55:11 GMT, Daniel Jeliński <djelinski at openjdk.org> wrote:

>> ### Description
>> Submitter reported when sending data of a length greater than 64kb using the `HttpClient` with HTTP/2, the HttpClient would hang indefinitely. Through reproducing the issue and analysing logs it was also seen that the HttpClient would not accept any incoming frames in this state. Note that 64kb is the default window size of a HTTP/2 Stream with the `HttpClient`.
>> 
>> The particular server used by the submitter did not emit window update frames to increase the size of the client's send window. As it stands now, when the client detects that sending more data will overflow the send window, it will await a window update from the remote receiver before attempting to transmit more data. If no window update comes from the server however, the client will not process any incoming frames from the server including RST_STREAM frames. It is important that if a remote indicates to the client that it can no longer process data with a RST_STREAM with the NO_ERROR flag set that it close the relevant connection instead of hanging indefinitely. See [section 8.1 of RFC 7540](https://www.rfc-editor.org/rfc/rfc7540#section-8.1) for description of this scenario.
>> 
>> ### Summary of Changes & Justification
>> The hanging of the client observed when a RST_STREAM is received is caused by the client not creating a responseSubscriber due to the response being seen as incomplete. The reason this happens is because in this case where the client cannot send more data due to the window being full, the request body is only partially finished sending. A responseSubscriber for reading the response from the server (i.e. any incoming frames) is only initialised when the request body has completed sending. However, this will not occur as the client is waiting for window updates.
>> 
>> If a RST_STREAM with NO_ERROR is observed, the request body will be completed without sending all of its data. This will initialise the responseSubcriber for the request and enable the client to process all incoming data from the client before subsequently processing the afforemntioned RST_STREAM frame.
>> 
>> ---------
>> ### Progress
>> - [ ] Change must be properly reviewed (1 review required, with at least 1 [Reviewer](https://openjdk.org/bylaws#reviewer))
>> - [x] Change must not contain extraneous whitespace
>> - [x] Commit message must refer to an issue
>> 
>> 
>> 
>> ### Reviewing
>> <details><summary>Using <code>git</code></summary>
>> 
>> Checkout this PR locally: \
>> `$ git fetch https://git.openjdk.org/jdk pull/12694/head:pull/12694` \
>> `$ git checkout pull/12694`
>> 
>> Update a local copy of the PR: \
>> `$ git checkout pull/12694` \
>> `$ git pull https://git.openjdk.org/jdk pull/12694/head`
>> 
>> </details>
>> <details><summary>Using Skara CLI tools</summary>
>> 
>> Checkout this PR locally: \
>> `$ git pr checkout 12694`
>> 
>> View PR using the GUI difftool: \
>> `$ git pr show -t 12694`
>> 
>> </details>
>> <details><summary>Using diff file</summary>
>> 
>> Download this PR as a diff file: \
>> <a href="https://git.openjdk.org/jdk/pull/12694.diff">https://git.openjdk.org/jdk/pull/12694.diff</a>
>> 
>> </details>
>
> src/java.net.http/share/classes/jdk/internal/net/http/Stream.java line 556:
> 
>> 554:             Flow.Subscriber<?> subscriber =
>> 555:                     responseSubscriber == null ? pendingResponseSubscriber : responseSubscriber;
>> 556:             if (frame.getErrorCode() == ResetFrame.NO_ERROR) {
> 
> We should stop sending regardless of the error code provided. 
> https://www.rfc-editor.org/rfc/rfc9113#name-rst_stream does not mention any special handling for specific error codes.

But only in case of NO_ERROR should we complete the requestBodyCF successfully.
In other cases, the reception of the reset frame should be considered as an error. 
There is also a subtle issue as to *when* the reset frame should be processed, and if it comes after we have started processing DataFrames we should not be processing it before we have processed the DataFrames that came before. These cases are handled below, in handleReset/receiveResetFrame.

@c-cleary could you please improve the comment here to explain what we're doing, and quote the relevant part of the spec (the paragraph that explains that a server may send a RESET frame with NO_ERROR *after* having sent the complete response if it doesn't need the request body to handle the response)?

-------------

PR: https://git.openjdk.org/jdk/pull/12694