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

[Daniel Fuchs]

On Tue, 15 Mar 2022 14:54:35 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 two additional commits since the last revision:
> 
>  - 8263031: Cleanup of changes in Http2Connection
>  - 8263031: Added test for multiple Continuation Frames

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

> 812:                     // TODO: Maybe say what kind of frame was received instead
> 813:                     pushContinuationState = null;
> 814:                     protocolError(ErrorFrame.PROTOCOL_ERROR, "Expected a Continuation frame but received " + frame);

In all other places in this method we have `return;` just after a call to `protocolError`, except in the two places where your changes added one. For consistency you should probably add this `return;` statement, even if it's not strictly needed. It would avoid having to have to analyze the whole structure of the nested `if - then - else` to figure out that it's actually not needed.

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

> 878:     private <T> void handlePushContinuation(Stream<T> parent, ContinuationFrame cf)
> 879:             throws IOException {
> 880:         decodeHeaders(cf, pushContinuationState.pushContDecoder);

I suggest declaring a local variable here to avoid reading pushContinuationState more than once.
Something like:


var pcs = pushContinuationState;

then use `pcs` wherever needed in that method.

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

> 73:     static HttpHeaders testHeaders;
> 74:     static HttpHeadersBuilder testHeadersBuilder;
> 75:     static int continuationCount;

Since these three static variables are set by one thread and read by another - they should all be volatile.

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

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