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

[Daniel Fuchs]

On Mon, 9 Oct 2023 10:22:39 GMT, Conor Cleary <ccleary at openjdk.org> 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