On Mon, 9 Oct 2023 10:22:39 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 ten additional
> commits since the last revision:
>
> - 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
> - 8309118: Remove local test timeout value
> - 8309118: Add more tests for 100 ExpectContinue with HTTP/2
src/java.net.http/share/classes/jdk/internal/net/http/Http2ClientImpl.java line
234:
> 232: connections.values().forEach(this::close);
> 233: connections.remove(entry.getKey(), entry.getValue());
> 234: }
I'd suggest to change `this::close(Http2Connection)` to return boolean (instead
of void) and make it always return`true`, and then use `removeIf` instead of
forEach:
connections.values().removeIf(this::close);
src/java.net.http/share/classes/jdk/internal/net/http/Stream.java line 193:
> 191: if (frame instanceof ResetFrame rf) {
> 192: inputQ.remove();
> 193: if (endStreamReceived()) {
Suggestion:
if (endStreamReceived() && rf.getErrorCode() ==
ResetFrame.NO_ERROR) {
src/java.net.http/share/classes/jdk/internal/net/http/Stream.java line 195:
> 193: if (endStreamReceived()) {
> 194: // If END_STREAM is already received, we should
> not receive any new RST_STREAM frames and
> 195: // close the connection gracefully by processing
> all remaining frames in the inputQ.
Suggestion:
// If END_STREAM is already received, we should we
should simply
// complete the requestBodyCF successfuly and stop
sending any
// request data.
src/java.net.http/share/classes/jdk/internal/net/http/Stream.java line 484:
> 482: if (debug.on()) debug.log("incoming: %s", frame);
> 483: var cancelled = checkRequestCancelled() || closed;
> 484: // endStreamSeen = endStreamSeen ||
> frame.getFlag(HeaderFrame.END_STREAM);
leftover to be removed?
src/java.net.http/share/classes/jdk/internal/net/http/Stream.java line 584:
> 582: void incoming_reset(ResetFrame frame) {
> 583: Log.logTrace("Received RST_STREAM on stream {0}", streamid);
> 584: Flow.Subscriber<?> subscriber = responseSubscriber == null ?
> pendingResponseSubscriber : responseSubscriber;
Suggestion:
Flow.Subscriber<?> subscriber = responseSubscriber;
if (subscriber == null) subscriber = pendingResponseSubscriber;
(avoids reading volatile twice)
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/15664#discussion_r1350308126
PR Review Comment: https://git.openjdk.org/jdk/pull/15664#discussion_r1350315812
PR Review Comment: https://git.openjdk.org/jdk/pull/15664#discussion_r1350312683
PR Review Comment: https://git.openjdk.org/jdk/pull/15664#discussion_r1350316732
PR Review Comment: https://git.openjdk.org/jdk/pull/15664#discussion_r1350319468
