8316580: HttpClient with StructuredTaskScope does not close when a task fails

[GoeLin]

I backport this for parity with 17.0.17-oracle.

The change applies clean, but the test does not work out-of-the-box.

It calls some utility methods from ReferenceTracker.java that were added by JDK-8305847.
That change is not in the current list of backports, so I added the utility methods
to this change.

Further, the test depends on the fact that in 21 a row of classes implement
AutoClosable, which they don't do in 21.
I adapted a row of places to work around this.
A try-with-resources statement was easy to replace, as the missing
close() implementation uses public methods, so that I could copy the
code to the test.
In TestTaskScope I replaced close() by shutdownNow(). Update: after some
research this was further adapted.
Finally I removed the "useReferenceTracker=false" test variant, as the
HttpClient implementation is quite far off of the implementation in 21,
so that the close() method implemented in 21 for Autoclosable can not
be easily simulated in the test.

Original test compile error:

test/jdk/java/net/httpclient/HttpGetInCancelledFuture.java:239: error: cannot find symbol
pool.close();
^
symbol: method close()
location: variable pool of type ExecutorService
test/jdk/java/net/httpclient/HttpGetInCancelledFuture.java:249: error: incompatible types: try-with-resources not applicable to variable type
try (final var executor = testExecutor()) {
^
(ExecutorService cannot be converted to AutoCloseable)
test/jdk/java/net/httpclient/HttpGetInCancelledFuture.java:277: error: incompatible types: Tracker cannot be converted to StringBuilder
System.out.println(TRACKER.diagnose(tracker));
^
test/jdk/java/net/httpclient/HttpGetInCancelledFuture.java:293: error: cannot find symbol
httpClient.close();
^
symbol: method close()
location: variable httpClient of type HttpClient

---

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
• JDK-8316580 needs maintainer approval

Issue

• JDK-8316580: HttpClient with StructuredTaskScope does not close when a task fails (Bug - P4)

Reviewers

• Paul Hohensee (@phohensee - Reviewer) 🔄 Re-review required (review applies to 622ef2fd)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk17u-dev.git pull/3706/head:pull/3706
$ git checkout pull/3706

Update a local copy of the PR:
$ git checkout pull/3706
$ git pull https://git.openjdk.org/jdk17u-dev.git pull/3706/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 3706

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk17u-dev/pull/3706.diff

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back goetz! 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]

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

[openjdk]

This backport pull request has now been updated with issue from the original commit.

[mlbridge]

Webrevs

• 02: Full - Incremental (9abc3cb0)
• 01: Full - Incremental (622ef2fd)
• 00: Full (1a29498b)

[GoeLin]

GHA failure: unrelated.
Error: Failed to FinalizeArtifact: Unable to make request: ECONNRESET

[openjdk]

⚠️ @GoeLin This change is now ready for you to apply for maintainer approval. This can be done directly in each associated issue or by using the /approval command.

[GoeLin]

I pushed an extra change to the test.
It replaces the shutdownNow() by more graceful coding.
This seems to better imitate the behavior of the original test.
Unfortunately with this change the test hangs until it is killed by jtreg (see attachted jtr file).
It only passes if run with the debug build, and once passed in our nightlies with the fastdebug.
I copied the modified test into my jdk21 repo where it passes as expected. Thus we think there is an issue with the backport in jdk, or the test unveils a further issue with the http implementation in 17.

HttpGetInCancelledFuture-stripped.jtr.txt

[GoeLin]

Hi @dfuch, @wxiao @djelinski
Could you please have a look at this backport? It seems I have missed something. The problem I describe on the comment above sounds a lot as if the original problem, not decrementing the refcount, still exists.
Obviously this backport is tricky as 17 lacks the virtual threads...
I would appreciate your help.
(I guessed the github name of you, Weibing Xiao. Maybe it works.)

[djelinski]

Hi @GoeLin, that's a different problem here; looking at the thread dump in the jtr file, you can see that the ForkJoinPool tasks are still running. Apparently cancel(true) does not interrupt the threads in JDK 17. I suppose the threads need to be interrupted explicitly.

[GoeLin]

Hi @djelinski, thanks for the help. We are now calling shutdownNow() in TestTaskScope.close(). The test results now look similar to 21.

[GoeLin]

Hi @alexeybakhtin, you did the open backport to 21 for this change. Could you please have a look at this backport and do a second review please? Thanks!