On Fri, 3 Feb 2023 12:49:47 GMT, Conor Cleary <[email protected]> wrote:
>>> The changes in this PR look good to me. While looking at the source code, I >>> noticed that the `Stream` class has a `PushedStream` nested class which too >>> has a similar implementation in `handleResponse()`. I haven't yet checked >>> the spec related to this; do you know if this code too will need a similar >>> fix? >> >> The original situation for a regular request-response in http2 was caused by >> a server responding to a client request with response headers that included >> only a code 200 status with no flags set and the related data. This was then >> followed by trailing headers with both the END_STREAM and END_HEADERS flags >> set with an empty header block (though the contents of the block aren't of >> concern here). >> >> Now, the Push Promise version of this situation is a bit different. A Push >> Promise frame is sent to the client containing all expected details such as >> the promisedStreamId, request path etc. However if we try mirror the >> previous situation by _not_ including an END_HEADERS flag in the Push >> Promise frame and attempting to send a trailing HEADERS frame, the >> HttpClient correcly throws a protocol error specifying that the client >> `Expected a Continuation frame but received HEADERS`. This is behavior >> defined in [section 6.6. of RFC7540: >> HTTP/2](https://www.rfc-editor.org/rfc/rfc7540#section-6.6). >> >> >> The PUSH_PROMISE frame defines the following flags: >> >> END_HEADERS (0x4): When set, bit 2 indicates that this >> frame contains an entire header block (Section 4.3) and >> is not followed by any CONTINUATION frames. >> >> A PUSH_PROMISE frame without the END_HEADERS flag >> set MUST be followed by a CONTINUATION frame for the >> same stream. A receiver MUST treat the receipt of any >> other type of frame or a frame on a different stream as a >> connection error (Section 5.4.1) of type PROTOCOL_ERROR. >> >> >> With this in mind, I would argue that in the case of >> `PushedStream::handleResponse` no changes are required as the behavior is >> quite well defined and strict when it comes to testing what the HttpClient >> will allow. One interesting side note is that END_STREAM is not strictly >> speaking a defined flag for PUSH_PROMISE frames. >> >> Interested to get some other opinions on that analysis or to see if there >> are any mistakes in that thinking, it was quite a tricky one to wrap my head >> around given the differences with how Push Requests are treated by the >> client. > >> The changes in this PR look good to me. While looking at the source code, I >> noticed that the `Stream` class has a `PushedStream` nested class which too >> has a similar implementation in `handleResponse()`. I haven't yet checked >> the spec related to this; do you know if this code too will need a similar >> fix? > > @jaikiran new changes have made modifications to > `PushedStream::handleResponse`. What follows is a short note on how this > differs from the changes with non-PushedStreams. > > A `PushedStream` contains a CompletableFuture `pushCF` which can finish in > one of two ways: > > - `pushCF.complete()` if a status code is included in response headers from > the server > - `pushCf.completeExceptionally()` if no status code included in server > response headers > > If any additional HEADERS frames (or frames of any other kind that are > permitted in this situation) are received, the only effect that these changes > will have is to produce a log stating the presence of trailing headers. It > will not cause `pushCF` to complete exceptionally as it has either already > done this or completed successfully. > > This differs from the implementation of `Stream::handleResponse` before the > changes made in this PR as Trailing headers without a status code would cause > an `IOException` to be thrown on the given response thread. > > ⚠️ @c-cleary This pull request contains merges that bring in commits not > > present in the target repository. Since this is not a "merge style" pull > > request, these changes will be squashed when this pull request in > > integrated. If this is your intention, then please ignore this message. If > > you want to preserve the commit structure, you must change the title of > > this pull request to `Merge <project>:<branch>` where `<project>` is the > > name of another project in the [OpenJDK > > organization](https://github.com/openjdk) (for example `Merge jdk:master`). > > ~This is due to local squashing of commits, can be ignored. I have verified > the correctness of the changeset~ > > Edit: On review, I think I really need to review this and make sure it will > behave well when merged. There may be a bit of messy activity here while I'm > cleaning this up Update, I beleive I was correct in in my original assessment and this was due to local squashing of commits already previously made. Apologies for the noise! I would ask for the comments between the quoted reply and this to be ignored as they are unrelated and were just used to sync the PRs changeset with the target branch (done switching target branch to an arbitrary one, then back to the master). All github information is telling me my changes can be auto-merged now so I am satisfied unless anyone spots anything. Otherwise I would direct reviewers to my earlier comment here > > The changes in this PR look good to me. While looking at the source code, I > > noticed that the `Stream` class has a `PushedStream` nested class which too > > has a similar implementation in `handleResponse()`. I haven't yet checked > > the spec related to this; do you know if this code too will need a similar > > fix? > > @jaikiran new changes have made modifications to > `PushedStream::handleResponse`. What follows is a short note on how this > differs from the changes with non-PushedStreams. > > A `PushedStream` contains a CompletableFuture `pushCF` which can finish in > one of two ways: > > * `pushCF.complete()` if a status code is included in response headers from > the server > * `pushCf.completeExceptionally()` if no status code included in server > response headers > > If any additional HEADERS frames (or frames of any other kind that are > permitted in this situation) are received, the only effect that these changes > will have is to produce a log stating the presence of trailing headers. It > will not cause `pushCF` to complete exceptionally as it has either already > done this or completed successfully. > > This differs from the implementation of `Stream::handleResponse` before the > changes made in this PR as Trailing headers without a status code would cause > an `IOException` to be thrown on the given response thread. ------------- PR: https://git.openjdk.org/jdk/pull/12028
