Re: RFR: 8284585: PushPromiseContinuation test fails intermittently in timeout [v3]
On Thu, 12 May 2022 11:54:55 GMT, Conor Cleary 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: Added test case to verify fix Test seems stable with latest changes after multiple runs, will integrate. - PR: https://git.openjdk.java.net/jdk/pull/8518
Re: RFR: 8284585: PushPromiseContinuation test fails intermittently in timeout [v3]
On Thu, 12 May 2022 11:54:55 GMT, Conor Cleary 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: Added test case to verify fix Please make sure the test is stable with these changes, and if it is then LGTM. - Marked as reviewed by dfuchs (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8518
Re: RFR: 8284585: PushPromiseContinuation test fails intermittently in timeout [v3]
> **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: Added test case to verify fix - Changes: - all: https://git.openjdk.java.net/jdk/pull/8518/files - new: https://git.openjdk.java.net/jdk/pull/8518/files/24e702a8..64fa937f Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8518=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8518=01-02 Stats: 81 lines in 3 files changed: 58 ins; 9 del; 14 mod Patch: https://git.openjdk.java.net/jdk/pull/8518.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8518/head:pull/8518 PR: https://git.openjdk.java.net/jdk/pull/8518
