RFR: 8284585: PushPromiseContinuation test fails intermittently in timeout [v2]

[Daniel Fuchs]

On Thu, 5 May 2022 13:37:08 GMT, Conor Cleary <ccleary at openjdk.org> wrote:

>> **Issue**
>> A recent fix for [JDK-8263031](https://bugs.openjdk.java.net/browse/JDK-8263031), which addressed the httpclient being unable to properly process Http/2 PushPromise frames followed by continuations caused intermittent failures of the test. This was cause by two seperate problems. 
>> 
>> - Firstly, on the client side, `Http2Connection.processFrame` did not a check for whether or not a Continuation was to be expected early on in the method resulting in frames other than the expected Continuation Frame being incorrectly processed. In Http/2, when a Continuation Frame is expected, no other type of frame should be processed without a connection error of type PROTOCOL_ERROR being thrown.
>> - Secondly, `Http2TestConnection.handlePush` had no guards around response headers and data frames being sent before a continuation is sent which resulted in the intermittent nature of this timeout. 
>> 
>> **Proposed Solution**
>> The test class `OutgoingPushPromise` was modified to contain a list of Continuation Frames as required rather than the Continuations being scheduled to send in the test itself. This prevents the situation of unexpected frames being sent before a Continuation Frame was meant to arrive. 
>> 
>> In addition to the above, Http2Connection was modified to ensure that a connection error of type PROTOCOL_ERROR is thrown if a frame other than a Continuation arrives at the client unexpectedly.
>
> Conor Cleary has updated the pull request incrementally with one additional commit since the last revision:
> 
>   8284585: New constructor & minor improvements

Changes requested by dfuchs (Reviewer).

test/jdk/java/net/httpclient/http2/server/OutgoingPushPromise.java line 41:

> 39:     final InputStream is;
> 40:     final int parentStream; // not the pushed streamid
> 41:     private final List<ContinuationFrame> continuations;

Looks much better - thanks. If you changed that to `List<Http2Frame>` instead it would allow you to add a test case where the server would send a regular HeaderFrame on the pushPromiseStream before sending the continuation on the original stream. You'd just have to put a HeaderFrame in that list before the ContinuationFrame. That would reproduce the failure we had before your fix, and that would allow you to verify that the bug that got the client hanging has been fixed on the client side too.

test/jdk/java/net/httpclient/http2/server/OutgoingPushPromise.java line 54:

> 52:         this.continuations = null;
> 53:     }
> 54: 

This constuctor should call the other consdtructor below:


    public OutgoingPushPromise(int parentStream,
                               URI uri,
                               HttpHeaders headers,
                               InputStream is) {
        this(parentStream, uri, headers, is, List.of());
    }

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

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