Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8309118: HttpClient: Add more tests for 100 ExpectContinue with HTTP/2 #15664

Closed
wants to merge 15 commits into from

Conversation

c-cleary
Copy link
Contributor

@c-cleary c-cleary commented Sep 11, 2023

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 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.

Note: The processing of a RST_STREAM is slightly varied based on whether or not the NO_ERROR code is used in the received RST_STREAM frame or any of the other codes are used.


Progress

  • Change must be properly reviewed (1 review required, with at least 1 Reviewer)
  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue

Issue

  • JDK-8309118: HttpClient: Add more tests for 100 ExpectContinue with HTTP/2 (Sub-task - P4)

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/15664/head:pull/15664
$ git checkout pull/15664

Update a local copy of the PR:
$ git checkout pull/15664
$ git pull https://git.openjdk.org/jdk.git pull/15664/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 15664

View PR using the GUI difftool:
$ git pr show -t 15664

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/15664.diff

Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented Sep 11, 2023

👋 Welcome back ccleary! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk
Copy link

openjdk bot commented Sep 11, 2023

@c-cleary The following label will be automatically applied to this pull request:

  • net

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added the net net-dev@openjdk.org label Sep 11, 2023
@c-cleary c-cleary changed the title 8309118: Add more tests for 100 ExpectContinue with HTTP/2 8309118: HttpClient: Add more tests for 100 ExpectContinue with HTTP/2 Sep 11, 2023
@c-cleary c-cleary marked this pull request as ready for review September 12, 2023 09:28
@openjdk openjdk bot added the rfr Pull request is ready for review label Sep 12, 2023
@mlbridge
Copy link

mlbridge bot commented Sep 12, 2023

http2WarmupURI = URI.create("http://" + http2TestServer.serverAuthority() + "/warmup");


h2resetStreamAfter100NoErrorUri = URI.create("http://" + http2TestServer.serverAuthority() + "/http2/resetStreamAfter100NoError");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
h2resetStreamAfter100NoErrorUri = URI.create("http://" + http2TestServer.serverAuthority() + "/http2/resetStreamAfter100NoError");
h2resetStreamAfter100NoErrorUri = URI.create("http://" + http2TestServer.serverAuthority() + "/http2/resetStreamAfter100NoError");

@Override
public void handle(Http2TestExchange exchange) throws IOException {
err.println("Sending 100");
exchange.sendResponseHeaders(100, 0);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
exchange.sendResponseHeaders(100, 0);
exchange.sendResponseHeaders(100, 0);

return new Object[][]{
// URI, Expected Status Code, Will finish with Exception, Protocol Version
{ postUri, 200, false, HTTP_1_1 },
{ hangUri, 417, false, HTTP_1_1},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
{ hangUri, 417, false, HTTP_1_1},
{ hangUri, 417, false, HTTP_1_1},

// URI, Expected Status Code, Will finish with Exception, Protocol Version
{ postUri, 200, false, HTTP_1_1 },
{ hangUri, 417, false, HTTP_1_1},
{ h2postUri, 200, false, HTTP_2 },
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
{ h2postUri, 200, false, HTTP_2 },
{ h2postUri, 200, false, HTTP_2 },

@c-cleary
Copy link
Contributor Author

Thanks for the reviews @turbanoff, I will adress them in a subsequent commit.

@c-cleary
Copy link
Contributor Author

c-cleary commented Sep 14, 2023

At the moment, I don't believe this is safe to commit. I'm seeing a very rare test timeout that could cause issues down the line. Specifically when the client hits the /resetStreamAfter200NoError URI, meaning that this failure is probably possible in the following /resetStreamAfter200Error case also. I'll be working to address this issue in due course.

Update: Seems purely related to the handling of the Error case and not the NoError case

I will also, in the same changeset, remove extraneous whitespace as highlighted and make a few stylistic updates (some of the variable names in tests are unnecessarily long, test data has unused parameters etc.)

@c-cleary
Copy link
Contributor Author

c-cleary commented Sep 20, 2023

Stability issues still present. Working on this, for the moment I pushed some cleanup changes highlighted by @turbanoff

Edit: A large overhaul of the changes to RST_STREAM handling and the test itself is taking place

@c-cleary
Copy link
Contributor Author

c-cleary commented Oct 9, 2023

This PR is now updated and ready for review again. A few fundamentals with regard to initial incorrect assumptions about the behaviour of HttpClient have been addressed. As a result the testing of these changes and the client in these RST_STREAM cases have changed quite a bit from the orignal posting. Fixes of various intermittent failures detected by running test cases included with this PR are also included. I will be updating the PR description to reflect this.

Edit: PR description is updated

Comment on lines 231 to 234
for (var entry : connections.entrySet()) {
connections.values().forEach(this::close);
connections.remove(entry.getKey(), entry.getValue());
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd suggest to change this::close(Http2Connection) to return boolean (instead of void) and make it always returntrue, and then use removeIf instead of forEach:

connections.values().removeIf(this::close);

Copy link
Contributor Author

@c-cleary c-cleary Oct 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So effect of your suggested changes is to remove the connection if close returns true as supposed to just removing all connections

This change is not entirely related to this PR. I will revert it and create a seperate issue. Though the suggestion is good and I will apply it to the give fix for that issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment on lines 194 to 195
// If END_STREAM is already received, we should not receive any new RST_STREAM frames and
// close the connection gracefully by processing all remaining frames in the inputQ.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// If END_STREAM is already received, we should not receive any new RST_STREAM frames and
// close the connection gracefully by processing all remaining frames in the inputQ.
// If END_STREAM is already received, we should we should simply
// complete the requestBodyCF successfuly and stop sending any
// request data.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about just

// If END_STREAM is already received, complete the requestBodyCF successfully
// and stop sending any request data.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

works for me

@@ -187,7 +190,13 @@ private void schedule() {
Http2Frame frame = inputQ.peek();
if (frame instanceof ResetFrame rf) {
inputQ.remove();
handleReset(rf, subscriber);
if (endStreamReceived()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (endStreamReceived()) {
if (endStreamReceived() && rf.getErrorCode() == ResetFrame.NO_ERROR) {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So if we have received an end stream but receive an error code, do we want to complete exceptionally?

The suggestion seems correct, just checking 👍

Copy link
Member

@dfuch dfuch Oct 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes - I believe so - the peer indicates an error - so we should complete exceptionally, if that's still possible.

void incoming_reset(ResetFrame frame) {
Log.logTrace("Received RST_STREAM on stream {0}", streamid);
if (endStreamReceived()) {
Flow.Subscriber<?> subscriber = responseSubscriber == null ? pendingResponseSubscriber : responseSubscriber;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Flow.Subscriber<?> subscriber = responseSubscriber == null ? pendingResponseSubscriber : responseSubscriber;
Flow.Subscriber<?> subscriber = responseSubscriber;
if (subscriber == null) subscriber = pendingResponseSubscriber;

(avoids reading volatile twice)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes, I see I think. In either case of the conditional expression either responseSubscriber and pendingResponseSubscriber are read or responseSubscriber is read and responseSubscriber is read. I was confused for a bit because I thought only one operand is evaluated. But actually its the case that responseSubscriber is read twice when responseSubscriber == null evaluates to false.

Good suggestion!

@@ -622,7 +632,7 @@ void handleReset(ResetFrame frame, Flow.Subscriber<?> subscriber) {
}
closed = true;
} finally {
stateLock.lock();
stateLock.unlock();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ouch! I am to blame for this one. It's a regression introduced by JDK-8308310. This one needs to be backported to JDK 21. Let me see if I should prepare a PR for that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@c-cleary
Copy link
Contributor Author

c-cleary commented Oct 9, 2023

Apologies for leaving those irrelevant comments and logs in, I tend to leave plenty of them around as I work to get a good idea of whats going on. I removed them all in the latest commit and will address the other comments you had @dfuch soon

@c-cleary
Copy link
Contributor Author

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.

Copy link
Member

@dfuch dfuch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines 618 to 619
if (!endStreamSeen) {
// If no END_STREAM flag seen, any RST_STREAM should be handled here immediately
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (!endStreamSeen) {
// If no END_STREAM flag seen, any RST_STREAM should be handled here immediately
if (!endStreamSeen || !finalResponseCodeReceived) {
// If no END_STREAM flag seen, any RST_STREAM should be handled here immediately
// If the final response code was not received, then we should also handle
// the RST_STREAM immediately

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So by checking if the final response code is received, this makes sure that the entire block of headers is processed therfore a status code will have been received.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And then in the case where END_STREAM has already been seen, we call requestBodyCF.complete(null); to complete normally, as any RST_STREAM frames after an END_STREAM are not handled exceptionally.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll give these changes a go and test them, will update here when I do

@c-cleary
Copy link
Contributor Author

c-cleary commented Oct 24, 2023

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). 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.

@c-cleary
Copy link
Contributor Author

Updated with your feedback @dfuch

Seems stable and makes sense 👍

@openjdk
Copy link

openjdk bot commented Oct 24, 2023

@c-cleary This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8309118: HttpClient: Add more tests for 100 ExpectContinue with HTTP/2

Reviewed-by: dfuchs, djelinski

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 186 new commits pushed to the master branch:

  • 744e089: 8318700: MacOS Zero cannot run gtests due to wrong JVM path
  • ec1bf23: 8318801: Parallel: Remove unused verify_all_young_refs_precise
  • 3cea892: 8318805: RISC-V: Wrong comments instructions cost in riscv.ad
  • bc1ba24: 8316437: JFR: assert(!tl->has_java_buffer()) failed: invariant
  • 970cd20: 8318788: java/net/Socks/SocksSocketProxySelectorTest.java fails on machines with no IPv6 link-local addresses
  • 37c40a1: 8318705: [macos] ProblemList java/rmi/registry/multipleRegistries/MultipleRegistries.java
  • 723db2d: 8305321: Remove unused exports in java.desktop
  • 811b436: 8318720: G1: Memory leak in G1CodeRootSet after JDK-8315503
  • a542f73: 8318843: ProblemList java/lang/management/MemoryMXBean/CollectionUsageThreshold.java in Xcomp
  • d96f38b: 8317510: Change Windows debug symbol files naming to avoid losing info when an executable and a library share the same name
  • ... and 176 more: https://git.openjdk.org/jdk/compare/37eb98604f4e2c241d959c7e2b337beb047421da...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Oct 24, 2023
@@ -201,7 +212,7 @@ private void schedule() {
connection.ensureWindowUpdated(df); // must update connection window
Log.logTrace("responseSubscriber.onComplete");
if (debug.on()) debug.log("incoming: onComplete");
sched.stop();
if (inputQ.isEmpty()) sched.stop();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you should remove all references to sched.stop() from this method; while adding if (inputQ.isEmpty()) reduces the risk of races, there's still a small window where an incoming reset frame can be lost.

Copy link
Member

@dfuch dfuch Oct 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only thing we could do with the reset after this point in the schedule() method, if the reset had not yet been inserted into the inputQueue, is to complete the requestBodyCF and this would have already been taken care of at line 595 in incoming_reset. So I don't believe the scheduler would need to run again after this line (or the other like it)

Copy link
Contributor Author

@c-cleary c-cleary Oct 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think maybe its better to let the scheduler complete without these checks. The effect of stop() is that it makes all subsequent calls to runOrSchedule() no-ops. It might be best to remove the call to stop() and allow scheduler to complete the requestBodyCf should a RST_STREAM be received in the small window in a similar manner to how its done in the line you referenced.

@dfuch
Copy link
Member

dfuch commented Oct 26, 2023

remove stop() from schedule()](37685cc)

With this change the scheduler may never get stopped. As long as we are sure that there won't be any subsequent call to schedule() (idle wake-ups), and that any non-idle wake-up would cause it to stop, it may be OK.

@c-cleary
Copy link
Contributor Author

c-cleary commented Oct 31, 2023

Thanks for the reviews! If there is no additional feedback, I will integrate these changes today.

@c-cleary
Copy link
Contributor Author

/integrate

@openjdk
Copy link

openjdk bot commented Oct 31, 2023

Going to push as commit 3a7525d.
Since your change was applied there have been 234 commits pushed to the master branch:

  • f4c5db9: 8318908: Parallel: Remove ExtendedCardValue
  • 7452d50: 8318940: [JVMCI] do not set HotSpotNmethod oop for a default HotSpotNmethod
  • 3e39d7b: 8319136: Skip pkcs11 tests on linux-aarch64
  • ee6f25b: 8319120: Unbound ScopedValue.get() throws the wrong exception
  • e05cafd: 8318467: [jmh] tests concurrent.Queues and concurrent.ProducerConsumer hang with 101+ threads
  • d3c3f0e: 8317951: Refactor loading of zip library to help resolve JDK-8315220
  • 576c9bc: 8318492: Http2ClientImpl should attempt to close and remove connection in stop()
  • 5411ad2: 8319106: Remove unimplemented TaskTerminator::do_delay_step
  • 75ce02f: 8318951: Additional negative value check in JPEG decoding
  • 328b381: 8009550: PlatformPCSC should load versioned so
  • ... and 224 more: https://git.openjdk.org/jdk/compare/37eb98604f4e2c241d959c7e2b337beb047421da...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Oct 31, 2023
@openjdk openjdk bot closed this Oct 31, 2023
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Oct 31, 2023
@openjdk
Copy link

openjdk bot commented Oct 31, 2023

@c-cleary Pushed as commit 3a7525d.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integrated Pull request has been integrated net net-dev@openjdk.org
4 participants