On Wed, 18 Oct 2023 10:19:51 GMT, Conor Cleary <[email protected]> wrote:
>> ### **Issue Description** >> There is missing test coverage for the implementation of >> partial/informational responses (status code 1xx in server response headers) >> for the `HttpClient`, specifically in the `HTTP/2` use case. RFC 9113 - >> HTTP/2, details the behavior of any client in these situations. Small >> changes and new test cases are needed to verify compliance with this >> specification. This issue primarily concerns how the client reacts to >> receiving a `RST_STREAM` frame at various stages of a partial/informational >> response from a server. >> >> ### **Solution Details** >> Changes were made in >> `src/java.net.http/share/classes/jdk/internal/net/http/Stream.java` to >> improve the handling client's handling of receiving a `RST_STREAM`. >> `incoming_reset()` (L574) will now cause the client to ignore a `RST_STREAM` >> frame if an `END_STREAM` flag has been received _and_ the request body has >> completed sending. Previously it would be ignored only if an `END_STREAM` >> flag was seen which caused cancelled partial responses to 'hang' >> indefinitely should a client be transmitting data in a POST/PUT request. An >> overhaul of how `incoming_reset()` was done in an effore to simplify the >> method and make it easier to debug in the future. >> >> Some changes where also made to the `schedule()` method in Stream (L190) to >> ensure both the sending of the Request Body and receipt of a RST_STREAM at >> various stages of an exchange do not cause unexpected behavior. Minor >> changes were made to the `Http2TestServer` implementation to improve the >> convinience of testing edge cases involving the sending of `HTTP/2` response >> headers. >> >> Concerning the new test cases, I have listed below the specifics of each >> case and the expected behavior of the client in the given scenario. >> >> **test/jdk/java/net/httpclient/ExpectContinueTest.java** >> - Client sends a POST request with the `Expect: 100-Continue` header included >> - Server responds with a `HEADERS` frame including a 100 status code. >> Server then sends `RST_STREAM` with code `NO_ERROR` or `PROTOCOL_ERROR` set >> before an END_STREAM is received from the server. >> - Expected/Observed Client Behavior: Client completes exceptionally >> in both cases. >> - Client sends a POST request with the `Expect: 100-Continue` header included >> - Server responds with a `HEADERS` frame including a 100 status code and >> reads the request body. Server then sends Response Headers with status code >> 200 to complete the response. Server then sends RST_STREAM with code `N... > > 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/1db564b9...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. ------------- PR Comment: https://git.openjdk.org/jdk/pull/15664#issuecomment-1768593234
