On Tue, 3 May 2022 15:00:51 GMT, Conor Cleary <ccle...@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. src/java.net.http/share/classes/jdk/internal/net/http/Http2Connection.java line 949: > 947: decrementStreamsCount(streamid); > 948: closeStream(streamid); > 949: System.err.println("closeStream"); Either remove or use a debug logger for this new trace. ------------- PR: https://git.openjdk.java.net/jdk/pull/8518
