RFR: 8263031: HttpClient throws Exception if it receives a Push Promise that is too large [v2]

[Daniel Fuchs]

On Mon, 7 Mar 2022 08:23:46 GMT, Conor Cleary <ccleary at openjdk.org> wrote:

>> **Problem**
>> When a Continuation Frame is received by the httpclient using HTTP/2 after a Push Promise frame (can happen if the amount of headers to be sent in a single Push Promise frame exceeds the maximum frame size, so a Continuation frame is required), the following exception occurs:
>> 
>> 
>> java.io.IOException: no statuscode in response
>> at java.net.http/jdk.internal.net.http.HttpClientImpl.send(HttpClientImpl.java:565)
>> at java.net.http/jdk.internal.net.http.HttpClientFacade.send(HttpClientFacade.java:119)
>> ...
>> 
>> This exception occurs because there is no existing flow in `jdk/internal/net/http/Http2Connection.java` which accounts for the case where a PushPromiseFrame is received with the END_HEADERS flag set to 0x0. When this occurs, the only acceptable frame/s (as multiple continuations are also acceptable) that can be received by the client on the same stream is a continuation frame.
>> 
>> **Fix**
>> To ensure correct behavior, the following changes were made to `jdk/internal/net/http/Http2Connection.java`.
>> 
>> - The existing method `handlePushPromise()` was modified so that if the END_HEADERS flag is _unset_ (flags equal to 0x0), then a record used to track the state of the Push Promise containing a shared `HeaderDecoder` and the received `PushPromiseFrame` is initialised.
>> - When the subsequent `ContinuationFrame` is received in `processFrame()`, the method `handlePushContinuation()` is called instead of the default flow resulting in `stream.incoming(frame)` being called (the source of the incorrect behaviour originally).
>> - In `handlePushContinuation()`, the shared decoder is used to decode the received `ContinuationFrame` headers and if the `END_HEADERS` flag is set (flags equal to 0x4), the `HttpHeaders` object for the Push Promise as a whole is constructed which serves to combine the headers from both the `PushPromiseFrame` and the `ContinuationFrame`.
>> 
>> A regression test was included which verifies that the exception is not thrown and that the headers arrive correctly.
>
> Conor Cleary has updated the pull request incrementally with one additional commit since the last revision:
> 
>   8263031: Tidied up import statements

src/java.net.http/share/classes/jdk/internal/net/http/Http2Connection.java line 82:

> 80: import java.util.function.Function;
> 81: import java.util.function.Supplier;
> 82: 

Could we keep the original alphabetical ordering and have the java.* imports come before the jdk.* imports?

src/java.net.http/share/classes/jdk/internal/net/http/Http2Connection.java line 780:

> 778: 
> 779:                 if (!(frame instanceof ResetFrame)) {
> 780:                     if (frame instanceof DataFrame) {

we could use instanceof pattern matching here to get rid of the cast

src/java.net.http/share/classes/jdk/internal/net/http/Http2Connection.java line 799:

> 797:             }
> 798: 
> 799:             // While pus frame is not null, the only acceptable frame on this

is there a letter missing in `pus` ?

src/java.net.http/share/classes/jdk/internal/net/http/Http2Connection.java line 803:

> 801:             if (pcs != null) {
> 802:                 if (frame instanceof ContinuationFrame cf) {
> 803:                     handlePushContinuation(stream, cf);

IOException should probably be caught and transformed in protocol error here to?
In case of protocol error when handling push promise (or their continuation) - should `pcs` be reset to null?
Maybe it doesn't matter since we're closing the connection anyway.

src/java.net.http/share/classes/jdk/internal/net/http/Http2Connection.java line 805:

> 803:                     handlePushContinuation(stream, cf);
> 804:                 } else {
> 805:                     // TODO: Maybe say what kind of frame was received instead

We should either do it or remove the TODO. @Michael-Mc-Mahon what would you suggest here?

src/java.net.http/share/classes/jdk/internal/net/http/Http2Connection.java line 855:

> 853: 
> 854:     private record PushContinuationState(HeaderDecoder pushContDecoder, PushPromiseFrame pushContFrame) {}
> 855:     private volatile PushContinuationState pcs;

I'd prefer to have a longer name than `pcs` - `pushContinuationState` would make it less mysterious at the place where it's used.  Also it could be good to move these declarations (linew 854 and 855) higher in the file, where other instance variables are declared.

test/jdk/java/net/httpclient/http2/PushPromiseContinuation.java line 25:

> 23: 
> 24: /*
> 25:  * @test

Please add an `@bug 8263031` tag here

test/jdk/java/net/httpclient/http2/PushPromiseContinuation.java line 33:

> 31:  *          java.net.http/jdk.internal.net.http.hpack
> 32:  * @run testng/othervm PushPromiseContinuation
> 33:  */

It would be good to add an @summary tag to explain the purpose of this test

test/jdk/java/net/httpclient/http2/PushPromiseContinuation.java line 176:

> 174:             ContinuationFrame cf = new ContinuationFrame(streamid, HeaderFrame.END_HEADERS, encodedHeaders);
> 175: 
> 176:             try {

It would be good to have a test-case that creates 2 continuation frames too.

test/jdk/java/net/httpclient/http2/PushPromiseContinuation.java line 231:

> 229:         }
> 230:     }
> 231: }

missing newline at end of file?

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

PR: https://git.openjdk.java.net/jdk/pull/7696