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

[Conor Cleary]

On Mon, 6 Mar 2023 17:59:12 GMT, Conor Cleary <ccleary 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>
>
> Conor Cleary has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains three additional commits since the last revision:
> 
>  - 8293786: Server-side reset implemented, updated comments on incoming_reset
>  - Merge remote-tracking branch 'upstream/master' into JDK-8293786
>  - 8293786: HttpClient will not send more than 64 kb of data from the 2nd request in http2

test/jdk/java/net/httpclient/lib/jdk/httpclient/test/lib/http2/Http2TestServerConnection.java line 735:

> 733:                 // isEof() for if END_STREAM has been seen.
> 734:                 // q.size() > 0 for if frames in Queue have not been read.
> 735:                 if (bis instanceof BodyInputStream inputStream && (!inputStream.isEof() || inputStream.q.size() > 0)) {

Caught an error with this code here when `bis` is assigned to `NullInputStream.INSTANCE` above. Need to put in an additional check for that.

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

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