On Tue, 22 Mar 2022 10:31:25 GMT, Conor Cleary <ccle...@openjdk.org> wrote:
>> **Problem** >> When a Continuation Frame is received by the httpclient using HTTP/2 after a >> Push Promise frame (can happen if the amount of headers to be sent in a >> single Push Promise frame exceeds the maximum frame size, so a Continuation >> frame is required), the following exception occurs: >> >> >> java.io.IOException: no statuscode in response >> at >> java.net.http/jdk.internal.net.http.HttpClientImpl.send(HttpClientImpl.java:565) >> at >> java.net.http/jdk.internal.net.http.HttpClientFacade.send(HttpClientFacade.java:119) >> ... >> >> This exception occurs because there is no existing flow in >> `jdk/internal/net/http/Http2Connection.java` which accounts for the case >> where a PushPromiseFrame is received with the END_HEADERS flag set to 0x0. >> When this occurs, the only acceptable frame/s (as multiple continuations are >> also acceptable) that can be received by the client on the same stream is a >> continuation frame. >> >> **Fix** >> To ensure correct behavior, the following changes were made to >> `jdk/internal/net/http/Http2Connection.java`. >> >> - The existing method `handlePushPromise()` was modified so that if the >> END_HEADERS flag is _unset_ (flags equal to 0x0), then a record used to >> track the state of the Push Promise containing a shared `HeaderDecoder` and >> the received `PushPromiseFrame` is initialised. >> - When the subsequent `ContinuationFrame` is received in `processFrame()`, >> the method `handlePushContinuation()` is called instead of the default flow >> resulting in `stream.incoming(frame)` being called (the source of the >> incorrect behaviour originally). >> - In `handlePushContinuation()`, the shared decoder is used to decode the >> received `ContinuationFrame` headers and if the `END_HEADERS` flag is set >> (flags equal to 0x4), the `HttpHeaders` object for the Push Promise as a >> whole is constructed which serves to combine the headers from both the >> `PushPromiseFrame` and the `ContinuationFrame`. >> >> A regression test was included which verifies that the exception is not >> thrown and that the headers arrive correctly. > > Conor Cleary has updated the pull request incrementally with four additional > commits since the last revision: > > - 8263031: Added return statements and pcs var > - 8263031: Test cleanup, added third case > - Revert "Test cleanup, added third case" > > This reverts commit 5c1c0f5fea801923bc3352bf88c701768d78b11d. > - Test cleanup, added third case Thanks for making these changes! Still a few comments... src/java.net.http/share/classes/jdk/internal/net/http/Http2Connection.java line 28: > 26: package jdk.internal.net.http; > 27: > 28: import jdk.internal.net.http.HttpConnection.HttpPublisher; Could this line be pushed down to where other `jdk.*` imports are made? test/jdk/java/net/httpclient/http2/PushPromiseContinuation.java line 158: > 156: CompletableFuture<HttpResponse<String>> cf = > 157: client.sendAsync(hreq, > HttpResponse.BodyHandlers.ofString(UTF_8), pph); > 158: cf.join(); I'd suggest checking that we received the expected response code + body too ------------- PR: https://git.openjdk.java.net/jdk/pull/7696
