8310645: CancelledResponse.java does not use HTTP/2 when testing the HttpClient #14625

c-cleary · 2023-06-23T12:13:47Z

Issue
In CancelledResponse.java the test only checks the HttpClient against HTTP/1.1 when cancelling a BodySubscriber while receiving data.

Solution
In the interest of more coverage, a new test has been created which performs the same checks against HTTP/2 to cover the client in the case of a HTTP/2 connection. A new test was created as it makes use of HttpTestServerAdapters to create the test servers. This is different to how this is performed in the original "CancelledResponse" test. There are some minor changes to how the testing is conducted with an element of randomness added to the new test.

As an open question to reviewers, the old test "CancelledResponse" and the new test "CancelledResponse2" could be merged into a single file and the HTTP/1.1 case could be updated to use more canonical testing methods as with "CancelledResponse2". Though there isn't a very pressing need for this and so it has not been included in this PR as of now.

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-8310645: CancelledResponse.java does not use HTTP/2 when testing the HttpClient (Bug - P4)

Reviewers

Daniel Fuchs (@dfuch - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 14625

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

Using diff file

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

Webrev

Link to Webrev Comment

… HttpClient

bridgekeeper · 2023-06-23T12:15:19Z

👋 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 · 2023-06-23T12:16:41Z

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

mlbridge · 2023-06-27T09:55:58Z

Webrevs

00: Full (30f3fcd5)

dfuch · 2023-06-27T11:02:48Z

test/jdk/java/net/httpclient/CancelledResponse2.java

+                cf.get();
+            } catch (Exception e) {
+                e.printStackTrace();
+                assertTrue(e.getCause() instanceof IOException, "HTTP/2 should cancel with an IOException when the Subscription is cancelled.");


I believe we want to check that the exception we get as the cause is the expected CancelException, or at least that it can be found somewhere while following the cause chain. Otherwise looks good!

So for HTTP/2, the cancelImpl() method in Stream looks like this.

@Override void cancel() { if ((streamid == 0)) { cancel(new IOException("Stream cancelled before streamid assigned")); } else { cancel(new IOException("Stream " + streamid + " cancelled")); } }

When the new IOException is passed to cancel, it sets the errorRef field to the IOException which then gets returned to us as the cause of the CompletableFuture's cause for cancelling. I think this behaviour is acceptable as the IOException returned will say "Stream x cancelled" which is accurate. Possible that I dont need to have this "CancelException" here

May be more difficult to verify the cause of cancellation beyond that which I'll think about.

There is a check that the cancelling BodyHandler sets the cancelled field which may be sufficient

Could also just check that the returned a call to getMessage() returns a string that contains cancelled but its not a very nice solution

Update: can verify the subscription is properly cancelled by adding a reference to the BodySubscriber used to the CancellingHandler (client) in the test. When the test is done, all thats needed is a call to SubscriptionWrapper.cancelled() to verify that the connection ended correctly.

dfuch · 2023-07-04T07:40:59Z

test/jdk/java/net/httpclient/CancelledResponse2.java

+                out.println("Cancelling subscription after reading " + index.get());
+                cancelled.set(true);
+                subscription.cancel();
+                result.completeExceptionally(new CancelException());


I believe that's the exception we expect to find in the HttpResponse. IIRC the original test was checking for that.

Right, but calling subscription.cancel() causes Stream.cancelImpl() to be called. That in turn causes the HttpResponse (or the variable result in your snippet above) to complete exceptionally with an IOException which has the message "Stream x cancelled". I think that means that the call to completeExceptionally(new CancelException()) has no effect because subscription.cancel() triggers a call to cancelImpl()

OK. LGTM then.

openjdk · 2023-07-04T13:18:45Z

@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:

8310645: CancelledResponse.java does not use HTTP/2 when testing the  HttpClient

Reviewed-by: dfuchs

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 147 new commits pushed to the master branch:

607ddaa: 8310997: missing @SInCE tags in jdk.httpserver
7655b48: 8311001: missing @SInCE info in jdk.net
711cddd: 8311249: Remove unused MemAllocator::obj_memory_range
514816e: 8309889: [s390] Missing return statement after calling jump_to_native_invoker method in generate_method_handle_dispatch.
60544f9: 8309894: compiler/vectorapi/VectorLogicalOpIdentityTest.java fails on SVE system with UseSVE=0
0916e6a: 8311092: Please disable runtime/jni/nativeStack/TestNativeStack.java on armhf
d8a0121: 8311109: tautological-compare warning in awt_Win32GraphicsDevice.cpp
b9198f9: 8254566: Clarify the spec of ClassLoader::getClassLoadingLock for non-parallel capable loader
f393975: 8310743: assert(reserved_rgn != nullptr) failed: Add committed region, No reserved region found
ba974d5: 8310661: JFR: Replace JVM.getJVM() with JVM
... and 137 more: https://git.openjdk.org/jdk/compare/370b8b2644e8951ba177c39199036c79ecf49401...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.

c-cleary · 2023-07-05T11:03:20Z

/integrate

openjdk · 2023-07-05T11:04:14Z

Going to push as commit 00ac46c.
Since your change was applied there have been 149 commits pushed to the master branch:

d6578bf: 8309531: Incorrect result with unwrapped iotaShuffle.
7b3c2dc: 8311122: Fix typos in java.base
607ddaa: 8310997: missing @SInCE tags in jdk.httpserver
7655b48: 8311001: missing @SInCE info in jdk.net
711cddd: 8311249: Remove unused MemAllocator::obj_memory_range
514816e: 8309889: [s390] Missing return statement after calling jump_to_native_invoker method in generate_method_handle_dispatch.
60544f9: 8309894: compiler/vectorapi/VectorLogicalOpIdentityTest.java fails on SVE system with UseSVE=0
0916e6a: 8311092: Please disable runtime/jni/nativeStack/TestNativeStack.java on armhf
d8a0121: 8311109: tautological-compare warning in awt_Win32GraphicsDevice.cpp
b9198f9: 8254566: Clarify the spec of ClassLoader::getClassLoadingLock for non-parallel capable loader
... and 139 more: https://git.openjdk.org/jdk/compare/370b8b2644e8951ba177c39199036c79ecf49401...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-07-05T11:04:24Z

@c-cleary Pushed as commit 00ac46c.

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

8310645: CancelledResponse.java does not use HTTP/2 when testting the…

30f3fcd

… HttpClient

c-cleary changed the title ~~8310645: CancelledResponse.java does not use HTTP/2 when testting the HttpClient~~ 8310645: CancelledResponse.java does not use HTTP/2 when testing the HttpClient Jun 23, 2023

openjdk bot added the net net-dev@openjdk.org label Jun 23, 2023

c-cleary marked this pull request as ready for review June 27, 2023 09:50

openjdk bot added the rfr Pull request is ready for review label Jun 27, 2023

dfuch reviewed Jun 27, 2023

View reviewed changes

dfuch reviewed Jul 4, 2023

View reviewed changes

dfuch approved these changes Jul 4, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jul 4, 2023

openjdk bot added the integrated Pull request has been integrated label Jul 5, 2023

openjdk bot closed this Jul 5, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jul 5, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8310645: CancelledResponse.java does not use HTTP/2 when testing the HttpClient #14625

8310645: CancelledResponse.java does not use HTTP/2 when testing the HttpClient #14625

c-cleary commented Jun 23, 2023 •

edited by openjdk bot

Loading

bridgekeeper bot commented Jun 23, 2023

openjdk bot commented Jun 23, 2023

mlbridge bot commented Jun 27, 2023

dfuch Jun 27, 2023 •

edited

Loading

c-cleary Jul 3, 2023 •

edited

Loading

c-cleary Jul 3, 2023

c-cleary Jul 3, 2023

c-cleary Jul 3, 2023

dfuch Jul 4, 2023

c-cleary Jul 4, 2023

dfuch Jul 4, 2023

openjdk bot commented Jul 4, 2023

c-cleary commented Jul 5, 2023

openjdk bot commented Jul 5, 2023

openjdk bot commented Jul 5, 2023

8310645: CancelledResponse.java does not use HTTP/2 when testing the HttpClient #14625

8310645: CancelledResponse.java does not use HTTP/2 when testing the HttpClient #14625

Conversation

c-cleary commented Jun 23, 2023 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Jun 23, 2023

openjdk bot commented Jun 23, 2023

mlbridge bot commented Jun 27, 2023

Webrevs

dfuch Jun 27, 2023 • edited Loading

Choose a reason for hiding this comment

c-cleary Jul 3, 2023 • edited Loading

Choose a reason for hiding this comment

c-cleary Jul 3, 2023

Choose a reason for hiding this comment

c-cleary Jul 3, 2023

Choose a reason for hiding this comment

c-cleary Jul 3, 2023

Choose a reason for hiding this comment

dfuch Jul 4, 2023

Choose a reason for hiding this comment

c-cleary Jul 4, 2023

Choose a reason for hiding this comment

dfuch Jul 4, 2023

Choose a reason for hiding this comment

openjdk bot commented Jul 4, 2023

c-cleary commented Jul 5, 2023

openjdk bot commented Jul 5, 2023

openjdk bot commented Jul 5, 2023

c-cleary commented Jun 23, 2023 •

edited by openjdk bot

Loading

dfuch Jun 27, 2023 •

edited

Loading

c-cleary Jul 3, 2023 •

edited

Loading