RFR: 8284585: PushPromiseContinuation test fails intermittently in timeout

[Daniel Fuchs]

On Tue, 3 May 2022 15:00:51 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.

Changes requested by dfuchs (Reviewer).

test/jdk/java/net/httpclient/http2/server/Http2TestServerConnection.java line 949:

> 947:         pp.streamid(op.parentStream);
> 948:         writeFrame(pp);
> 949:         if (pp.getFlags() != HeadersFrame.END_HEADERS && op.hasContinuations()) {

Maybe you could just drop `op.hasContinuations()`

test/jdk/java/net/httpclient/http2/server/Http2TestServerConnection.java line 950:

> 948:         writeFrame(pp);
> 949:         if (pp.getFlags() != HeadersFrame.END_HEADERS && op.hasContinuations()) {
> 950:             LinkedList<ContinuationFrame> continuations = new LinkedList<>(op.getContinuations());

That copy seems unneeded?

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

> 37:     final InputStream is;
> 38:     final int parentStream; // not the pushed streamid
> 39:     private LinkedList<ContinuationFrame> continuations;

Make this `final` and instantiate it in the constructor instead. 
Could use `List<ContinuationFrame>` here - or even `List<Http2Frame>` if you want to simulate the bug we had before - e.g - you could add a `HeaderFrame` instead of a `ContinuationFrame`, and verify the client no longer hangs and rightfully closes the connection...

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

> 54:             continuations = new LinkedList<>();
> 55:         continuations.add(cf);
> 56:     }

I would suggest adding a constructor that takes a list of continuations/frames instead. This way you could have an immutable list (use List.of/List.copyOf)

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

> 58:     public boolean hasContinuations() {
> 59:         return !continuations.isEmpty();
> 60:     }

That method is not needed. Plus it will throw if `continuations` is null.

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

> 62:     public LinkedList<ContinuationFrame> getContinuations() {
> 63:         return continuations;
> 64:     }

Same remark here - you could use `List` instead of `LinkedList`.

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

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