On Wed, 18 Oct 2023 14:30:28 GMT, Conor Cleary <[email protected]> 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
