On Tue, 7 Feb 2023 09:49:12 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? >> >> @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 by 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. > @c-cleary Please do not rebase or force-push to an active PR as it > invalidates existing review comments. Note for future reference, the bots > always squash all changes into a single commit automatically as part of the > integration. See [OpenJDK Developers’ > Guide](https://openjdk.org/guide/#working-with-pull-requests) for more > information. Had a recent accidental commit, removing produced this message hence the noise. ------------- PR: https://git.openjdk.org/jdk/pull/12028
