RFR: 8296410: HttpClient throws java.io.IOException: no statuscode in response for HTTP2 [v2]

[Daniel Fuchs]

On Fri, 3 Feb 2023 12:34:26 GMT, Conor Cleary <ccleary at openjdk.org> wrote:

>> ### **Description**
>> With HTTP/2, a possible flow of an exchange could be that a Server responds to a simple request by sending response headers within a HEADERS frame, the requested resources and trailing headers with another HEADERS frame including the END_HEADERS and END_STREAM flags (if they were not previously given in the Response Headers). Usually an HTTP/2 client should opt-in to make use of trailing headers though this behaviour is not supoorted by default with the HttpClient. 
>> 
>> It is still possible in any case that a trailing HEADERS frame can be sent, even if the frame just carries flags and no headers specified in the header block. This edge case was causing an IOException to be thrown and the client to shut down unexpectedly.
>> 
>> ### **Summary of Changes & Justification**
>> In Stream, a means to track the state of whether or not a final status code for an exchange has been received was added to prevent the exception occuring due to a HEADERS frame with no status code arriving after response headers were received. In the case of a partial response (status codes in the 1XX range), the flag for receiving a final response code is not set until a response outside of the 1XX range is received.
>> 
>> Lastly, if trailing headers are received, they are logged and dumped. The response headers consumer is then reset.
>> 
>> ---------
>> ### 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/12028/head:pull/12028` \
>> `$ git checkout pull/12028`
>> 
>> Update a local copy of the PR: \
>> `$ git checkout pull/12028` \
>> `$ git pull https://git.openjdk.org/jdk pull/12028/head`
>> 
>> </details>
>> <details><summary>Using Skara CLI tools</summary>
>> 
>> Checkout this PR locally: \
>> `$ git pr checkout 12028`
>> 
>> View PR using the GUI difftool: \
>> `$ git pr show -t 12028`
>> 
>> </details>
>> <details><summary>Using diff file</summary>
>> 
>> Download this PR as a diff file: \
>> <a href="https://git.openjdk.org/jdk/pull/12028.diff">https://git.openjdk.org/jdk/pull/12028.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 eight additional commits since the last revision:
> 
>  - Merge branch 'JDK-8296410' of https://github.com/c-cleary/jdk into JDK-8296410
>  - 8296410: Updated copyright header
>  - 8296410: Use empty trailers instead of sample ones
>  - 8296410: HttpClient throws java.io.IOException: no statuscode in response for HTTP2
>  - 8296410: Made test compatible with new test library structure
>  - Merge branch 'master' of https://github.com/c-cleary/jdk into JDK-8296410
>  - 8296410: Push Promise Test Case Modifications
>  - 8296410: HttpClient throws java.io.IOException: no statuscode in response for HTTP2

Marked as reviewed by dfuchs (Reviewer).

src/java.net.http/share/classes/jdk/internal/net/http/Stream.java line 1501:

> 1499:                 }
> 1500: 
> 1501:                 this.finalPushResponseCodeReceived = true;

So the assumption here is that it doesn't make sense for a push promise response to have an intermediary 1xx code - so we don't support that and just return the first response we receive. I believe that's right. There may be a followup issue to log though, which would be to write test, and possibly revisit this code, to verify that we do fail in cases where we receive an unexpected sequence of frames, such as for instance, two final status codes, or several trailing header frames, or trailing headers followed by DataFrames (and this both on regular streams and pushed streams). However, this would be better handled in a followup PR. Maybe we could also investigate (in that followup) whether we should fail here if we receive a 1xx - instead of just returning it (on the other hand, maybe returning it and ignoring whatever follows is the right thing to do?) @Michael-Mc-Mahon what would you advise?

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

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