8292381: java/net/httpclient/SpecialHeadersTest.java fails with "ERROR: Shutting down connection: HTTP/2 client stopped" #9908

dfuch · 2022-08-17T17:06:12Z

Please find here a change that improves SpecialHeadersTest. This test creates a large amount of ephemeral clients and has been observed running out of heap space in our CI once. This change updates the test to wait for the previous HttpClient to be eligible for garbage collection before it creates a new one. It also verifies that no outstanding operation are still running on the client by the time the client is released.

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-8292381: java/net/httpclient/SpecialHeadersTest.java fails with "ERROR: Shutting down connection: HTTP/2 client stopped"

Reviewers

Jaikiran Pai (@jaikiran - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 9908

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

Using diff file

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

…R: Shutting down connection: HTTP/2 client stopped"

bridgekeeper · 2022-08-17T17:06:29Z

👋 Welcome back dfuchs! 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 · 2022-08-17T17:12:22Z

@dfuch 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 · 2022-08-17T17:16:39Z

Webrevs

01: Full - Incremental (cd588b71)
00: Full (3d951b08)

jaikiran · 2022-08-18T06:41:49Z

test/jdk/java/net/httpclient/HttpServerAdapters.java

+        public boolean equals(Object o) {
+            if (this == o) return true;
+            if (!(o instanceof HttpTestRequestHeaders headers)) return false;
+            return Objects.equals(entrySet(), headers.entrySet());


This code confused me a bit initially. Each of the subclasses of HttpTestRequestHeaders has a headers private member and I misread this line to be comparing the headers entrySet with itself. Would it be better if we renamed this local variable (in the instanceof line above) to otherHeaders to avoid any confusion?

Why doesn't this comment appears when I click on the "Files Changed" tab? Strange.
Anyway - yes - good point - will change headers to other - I agree it's confusing.

jaikiran · 2022-08-18T06:52:41Z

test/jdk/java/net/httpclient/SpecialHeadersTest.java

+                    tasksFailed = true;
+                    out.printf(now() + "Task %s failed: %s%n", id, t);
+                    err.printf(now() + "Task %s failed: %s%n", id, t);
+                    FAILURES.putIfAbsent("Task " + id, t);


Hello Daniel, is the putIfAbsent intentional here? I wouldn't expect the task id to repeat here (or would there be that many tasks that the long id value overflows and repeats?).

It's copy-pasted from other tests that use the same technique. Mainly we want to make sure we get the first error. I agree that putIfAbsent might not been necessary but it doesn't hurt either: it makes it clear that the value associated with the key will not be overridden.

jaikiran · 2022-08-18T07:06:52Z

test/jdk/java/net/httpclient/SpecialHeadersTest.java

+                // expectation that the first HTTP/2 clear request
+                // will be an upgrade
+                if (shared != null) {
+                    TRACKER.track(shared);


Is it intentional that we track a shared client when it is being reset, instead of tracking it when we create it (a few lines later)? This is unlike a non-shared instance which we track when we create one.

Yes - because the tracker will complain if any of the tracked client is still alive when check() is called. So the idea is to start tracking the shared client when we're no longer using it.

jaikiran · 2022-08-18T07:16:39Z

test/jdk/java/net/httpclient/SpecialHeadersTest.java

+                    client = null;
+                    System.gc();
+                    var error = TRACKER.check(500);
+                    if (error != null) throw error;


Given slowness on CI systems, would it be better to use the jtreg timeout factor here (and other similar places in other test methods) to adjust the 500ms timeout? Or better still, perhaps just log a warning instead of throwing an error? After all this test's main goal is to verify the request headers and in that context it should be OK if some HttpClient(s) aren't gc collected by the time the test completes?

I haven't observed any issue yet with using a 500ms. Here or in other tests. And I do want to throw an error if a client doesn't cleanly exits. So if we get a failure here - we should look at what prevented the client from exiting - and if it looks like it might be that the GC didn't get enough cycle then we would raise the timeout.

jaikiran · 2022-08-18T07:26:16Z

test/jdk/java/net/httpclient/SpecialHeadersTest.java

-                .proxy(NO_PROXY)
-                .sslContext(sslContext)
-                .build();
+        HttpClient client = newHttpClient("testHomeMadeIllegalHeader", sameClient);


Previously before this change, this test method wasn't using the sameClient param while creating the new client and instead was creating it always. I don't know if that was intentional (in fact it wasn't even using the headerNameAndValue param being passed to it). Is this change to use the sameClient param while creating this client here, intentional?

yes - before that the method would be running twice with a brand new client. I believe that was an oversight. I don't think that sameClient=true|false matters much here, which probably explains why the sameClient parameters was ignored. We could create a special provider that doesn't pass sameClient, but since this method should be quite short I decided to simply change it to take the value of sameClient into account.

jaikiran · 2022-08-18T07:31:22Z

Additionally, the SpecialHeadersTest would need a copyright year update and given the kind of changes we are doing in this PR, would it even warrant a @bug id inclusion?

jaikiran · 2022-08-18T07:37:41Z

Overall, I understand these changes but I'm still a bit surprised with the way this test failed on the CI system especially after how we try to stop/close the relevant resources (like the SelectorManager thread) as soon as we realize the client is no longer used and doesn't have any pending operations.

dfuch · 2022-08-18T09:23:10Z

Additionally, the SpecialHeadersTest would need a copyright year update and given the kind of changes we are doing in this PR, would it even warrant a @bug id inclusion?

This is a test bug. We only add a @bug for test changes that test source code changes. Thanks for noticing the copyright. Will fix that.

dfuch · 2022-08-18T09:26:24Z

Overall, I understand these changes but I'm still a bit surprised with the way this test failed on the CI system especially after how we try to stop/close the relevant resources (like the SelectorManager thread) as soon as we realize the client is no longer used and doesn't have any pending operations.

Yes I agree the OOM was surprising. We never had that before and we didn't change anything in the client recently. Maybe that was a new machine with lower specs. Making these tests less greedy in resources is a worthwhile goal however.

Michael-Mc-Mahon

I don't really understand why the test (originally) needed to create so many clients. Wouldn't it be easier and more maintainable to only create the minimum number of clients?

dfuch · 2022-08-18T10:11:45Z

I don't really understand why the test (originally) needed to create so many clients. Wouldn't it be easier and more maintainable to only create the minimum number of clients?

Yeah - maybe. But there are differences between a brand new client and 'used' client: one may go through the upgrade when using h2c where the other might not because it might already have a connection opened. These tests are what there are. They did help flush a number of race conditions over the time that we might not have seen if we had simpler tests.

dfuch · 2022-08-19T10:47:04Z

I pushed the requested changes to the PR branch.

jaikiran

Thank you for the changes, Daniel. Looks fine to me.

openjdk · 2022-08-19T13:16:26Z

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

8292381: java/net/httpclient/SpecialHeadersTest.java fails with "ERROR: Shutting down connection: HTTP/2 client stopped"

Reviewed-by: jpai

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

37aa7c1: 8292559: Add test for -XX:+CheckJNICalls showing changed signal handlers
f2f0cd8: 8292511: AArch64: Align CPU feature name for NEON with hwcap
1f484da: 8289562: Change bugs.java.com and bugreport.java.com URL's to https
63a126a: 8292607: Remove unused dirty and dirty_card_range_after_reset in CardTable
7d18ebd: 8292606: G1 and Epsilon header cleanup for JDK-8282729
2edd550: 8292312: Work around memset() called operator new
964aac2: 8292499: CDS ArchivedEnumTest.java fails: object points to a static field that may be reinitialized
f85411f: 8292458: Atomic operations on scoped enums don't build with clang
82dbe29: 8292633: runtime/cds/appcds/dynamicArchive/CDSStreamTestDriver.java fails to compile
54ce114: 6587699: Document DigestInputStream behavior when skip() or mark() / reset() is used
... and 17 more: https://git.openjdk.org/jdk/compare/e81210f5fe03ea3dc9c9fb0dba2be79e1dcc03bc...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.

dfuch · 2022-08-22T14:13:05Z

/integrate

openjdk · 2022-08-22T14:14:48Z

Going to push as commit 256b523.
Since your change was applied there have been 56 commits pushed to the master branch:

e561933: 8292623: Reduce runtime of java.io microbenchmarks
dcd7802: 8292708: Rename G1ParScanThreadState::flush to flush_stats
16593cf: 8292717: Clean up checking of testing requirements in configure
c59f9b3: 8287828: Fix so that one can select jtreg test case by ID from make
476c484: 8292656: G1: Remove G1HotCardCache::_use_cache
a17fce7: 6445283: ProgressMonitorInputStream not large file aware (>2GB)
1ed03d8: 8292226: Prepare make for better Link Time Optimization support
79597f1: 8272702: Resolving URI relative path with no / may lead to incorrect toString
7b5f9ed: 8288966: Better handle very spiky promotion in G1
07c7977: 8290249: Vectorize signum on AArch64
... and 46 more: https://git.openjdk.org/jdk/compare/e81210f5fe03ea3dc9c9fb0dba2be79e1dcc03bc...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-08-22T14:15:05Z

@dfuch Pushed as commit 256b523.

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

8292381: java/net/httpclient/SpecialHeadersTest.java fails with "ERRO…

3d951b0

…R: Shutting down connection: HTTP/2 client stopped"

openjdk bot added the rfr Pull request is ready for review label Aug 17, 2022

openjdk bot added the net net-dev@openjdk.org label Aug 17, 2022

jaikiran reviewed Aug 18, 2022

View reviewed changes

Michael-Mc-Mahon reviewed Aug 18, 2022

View reviewed changes

Integrated review feedback

cd588b7

jaikiran approved these changes Aug 19, 2022

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Aug 19, 2022

openjdk bot added the integrated Pull request has been integrated label Aug 22, 2022

openjdk bot closed this Aug 22, 2022

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Aug 22, 2022

dfuch deleted the SpecialHeadersTest-8292381 branch November 22, 2022 17:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8292381: java/net/httpclient/SpecialHeadersTest.java fails with "ERROR: Shutting down connection: HTTP/2 client stopped" #9908

8292381: java/net/httpclient/SpecialHeadersTest.java fails with "ERROR: Shutting down connection: HTTP/2 client stopped" #9908

dfuch commented Aug 17, 2022 •

edited by openjdk bot

Loading

bridgekeeper bot commented Aug 17, 2022

openjdk bot commented Aug 17, 2022

mlbridge bot commented Aug 17, 2022 •

edited

Loading

jaikiran Aug 18, 2022

dfuch Aug 18, 2022

jaikiran Aug 18, 2022

dfuch Aug 18, 2022

jaikiran Aug 18, 2022

dfuch Aug 18, 2022

jaikiran Aug 18, 2022 •

edited

Loading

dfuch Aug 18, 2022

jaikiran Aug 18, 2022

dfuch Aug 18, 2022

jaikiran commented Aug 18, 2022

jaikiran commented Aug 18, 2022

dfuch commented Aug 18, 2022 •

edited

Loading

dfuch commented Aug 18, 2022 •

edited

Loading

Michael-Mc-Mahon left a comment

dfuch commented Aug 18, 2022

dfuch commented Aug 19, 2022

jaikiran left a comment

openjdk bot commented Aug 19, 2022

dfuch commented Aug 22, 2022

openjdk bot commented Aug 22, 2022

openjdk bot commented Aug 22, 2022

8292381: java/net/httpclient/SpecialHeadersTest.java fails with "ERROR: Shutting down connection: HTTP/2 client stopped" #9908

8292381: java/net/httpclient/SpecialHeadersTest.java fails with "ERROR: Shutting down connection: HTTP/2 client stopped" #9908

Conversation

dfuch commented Aug 17, 2022 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Aug 17, 2022

openjdk bot commented Aug 17, 2022

mlbridge bot commented Aug 17, 2022 • edited Loading

Webrevs

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaikiran Aug 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaikiran commented Aug 18, 2022

jaikiran commented Aug 18, 2022

dfuch commented Aug 18, 2022 • edited Loading

dfuch commented Aug 18, 2022 • edited Loading

Michael-Mc-Mahon left a comment

Choose a reason for hiding this comment

dfuch commented Aug 18, 2022

dfuch commented Aug 19, 2022

jaikiran left a comment

Choose a reason for hiding this comment

openjdk bot commented Aug 19, 2022

dfuch commented Aug 22, 2022

openjdk bot commented Aug 22, 2022

openjdk bot commented Aug 22, 2022

dfuch commented Aug 17, 2022 •

edited by openjdk bot

Loading

mlbridge bot commented Aug 17, 2022 •

edited

Loading

jaikiran Aug 18, 2022 •

edited

Loading

dfuch commented Aug 18, 2022 •

edited

Loading

dfuch commented Aug 18, 2022 •

edited

Loading