RFR: 8296410: HttpClient throws java.io.IOException: no statuscode in response for HTTP2

[Conor Cleary]

On Tue, 7 Feb 2023 09:49:12 GMT, Conor Cleary <ccleary at openjdk.org> 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