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

[Conor Cleary]

On Wed, 22 Feb 2023 13:08:00 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>
>
> test/jdk/java/net/httpclient/http2/PostPutTest.java line 109:
> 
>> 107:     }
>> 108: 
>> 109:     static class PostPutExchange extends Http2TestExchangeImpl {
> 
> Can we make the HTTP2 server always reset the stream if the configured Http2Handler doesn't consume the entire input?
> There's even a TODO for that:
> https://github.com/openjdk/jdk/blob/master/test/jdk/java/net/httpclient/lib/jdk/httpclient/test/lib/http2/BodyInputStream.java#L133

Update on this, the issue here is making sure that the reset frame on the server side is triggered in the correct manner for an individual BodyInputStream instance. This can be done by checking various conditions such as the stream having unprocessed/read frames in it's Queue (in other words checking if the entire input has not been consumed highlighted by @djelinski above) and making sure that a reset frame is not incorrectly sen after receiving a connection preface, settings etc.

I've almost got this sorted though, so hopefully new changes here shortly

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

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