RFR: 8284585: PushPromiseContinuation test fails intermittently in timeout

[Conor Cleary]

On Wed, 4 May 2022 11:05:56 GMT, Daniel Fuchs <dfuchs 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.
>
> src/java.net.http/share/classes/jdk/internal/net/http/Http2Connection.java line 881:
> 
>> 879:             nextPushStream += 2;
>> 880:         }
>> 881: 
> 
> Hi Conor - sorry for suggesting to move this code up into `handlePushPromise`. Now that I see the whole picture I believe these lines should have stayed where they were, in `completePushPromise`. The main reason is that we need to properly identify and decode possible ContinuationFrames as being part of the push promise in order to maintain the HPack encoders/decoders in sync, and we can only do that if we keep the `pushContinuationState` in place until we have received the END_HEADERS flag.
> 
> Sorry for suggesting that wrong move - it was my mistake.

No problem at all Daniel. I see what you're saying, essentially that if the Continuation does come in on the wrong stream id, it still needs to be decoded and identified as a part of the Push Promise before being rejected with the wrong stream id.

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

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