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. 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
