RFR: 8309118: HttpClient: Add more tests for 100 ExpectContinue with HTTP/2 [v5]

[Conor Cleary]

On Wed, 18 Oct 2023 14:30:28 GMT, Conor Cleary <ccleary at openjdk.org> wrote:

>> Conor Cleary has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 12 additional commits since the last revision:
>> 
>>  - 8309118: Update Comment in schedule()
>>  - Merge remote-tracking branch 'upstream/master' into JDK-8309118
>>  - 8309118: Remove irrelevant comments and logs
>>  - 8309118: Fixed error in handleReset ReentrantLock
>>  - 8309118: Updates to Stream cancelImpl
>>  - 8309118: Refactored test, updated incoming_reset
>>  - Merge branch 'master' into JDK-8309118
>>  - 8309118: Cleanup identifiers and whitespace
>>  - 8309118: Removed unused try-with-resources
>>  - 8309118: Improve comments and annotations
>>  - ... and 2 more: https://git.openjdk.org/jdk/compare/86e9cb03...d74b08a0
>
> This PR has generated a lot of discussion offline. If I were to summarise it comes down to a couple of points. `pendingResponseSubscriber` can be null when `receiveDataFrame(new DataFrame(streamid, DataFrame.END_STREAM, List.of()));` is called because `readBodyAsync` has not yet been called (which triggers the setting of `pendingResponseSubsrciber`). There was a lot of confusion around this but because it is necessary to work around the constraints of the `Exchange` class, this seems alright. `readBodyAsync` won't happen until data is received from the server after the client has received response headers. If there were any **actions** to come from this it would be to improve comments around this to make this more clear.
> 
> The new `endStreamSeenFlag` is used very specifically in my changes in two cases of processing a RST_STREAM frame from the server. Firstly, in the event the `pendingResponseSubscriber` is null (as in the case described above) and an END_STREAM flag has not been received by the client (so `endStreamSeen` is `false`) `handleReset` is called then and there to complete the `requestBodyCf` exceptionally. Secondly, in the event of `requestBodyCF` not being done or we are still sending the request body, `handleReset` is called straight away here too. So in essence its being used as a short-circuit to complete the requestBodyCf exceptionally. I would like to seek feedback on this specifically if there is any.

> Hi @c-cleary this mostly look good to me now. I do have one additional suggestion though. There is a corner case where we may have received END_STREAM but are still expecting some frames: that's the case of CONTINUATION frames: in that case the END_STREAM is carried by the HEADERS frame, and the END_HEADERS will be carried by a continuation frame that follows. If we receive a RESET at that point, we should also handle it immediately and relay an exception to the caller. I believe that could be easily handled by handling reset immediately also in the case where `finalResponseCodeReceived` is `false`. See suggestion below.

Ah yes, I see this text now in RFC 9113 ([link here](https://www.rfc-editor.org/rfc/rfc9113.html#section-6.2)). Quote from RFC..

> A HEADERS frame with the END_STREAM flag set signals the end of a stream. However, a HEADERS frame with the END_STREAM flag set can be followed by CONTINUATION frames on the same stream. Logically, the CONTINUATION frames are part of the HEADERS frame.

-------------

PR Comment: https://git.openjdk.org/jdk/pull/15664#issuecomment-1776860554