8317808: HTTP/2 stream cancelImpl may leave subscriber registered

[dfuch]

Some of the ThrowingSubscriberXxx tests (typically those that use a streaming body) have been observed failing intermittently (though rarely) in timeout. The error message printed at the end showed that some HTTP/2 stream subscribers had not been properly deregistered when an exception was thrown in GET_BODY.

The issue appears to be a race condition between cancelImpl and schedule where the scheduler sched may get stopped before the subscriber is exceptionally completed.

A simple fix is to ensure that cancelImpl will complete the subscriber, if needed.

---

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-8317808: HTTP/2 stream cancelImpl may leave subscriber registered (Bug - P4)

Reviewers

• Michael McMahon (@Michael-Mc-Mahon - Reviewer)
• Daniel Jeliński (@djelinski - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 16144

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

Using diff file

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

Webrev

Link to Webrev Comment

[bridgekeeper]

👋 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]

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

Webrevs

• 01: Full - Incremental (a97ebe20)
• 00: Full (4cc23df3)

[Michael-Mc-Mahon]

Is this change here actually part of the fix, or something else that needed to be fixed?

[dfuch]

A bit of both. I was looking at why the subscriber didn't get properly unregistered and stumbled on this small issue. Tests showed that it wasn't sufficient to fix the issue so I continued debugging.
The semantics of the method is that it should return true only if the subscriber was added to the list, so it really should have returned false here, and true only if subscribers.add was called and returned true.

[Michael-Mc-Mahon]

Seems straight forward. LGTM

[openjdk]

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

8317808: HTTP/2 stream cancelImpl may leave subscriber registered

Reviewed-by: michaelm, 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 42 new commits pushed to the master branch:

• 7d31146: 8318028: Remove unused class="centered" from FocusCycle.svg
• 45b7aed: 8317433: Async UL: Only grab lock once when write():ing
• 266636d: 8315940: ARM32: Move field resolution information out of the cpCache
• e490cf9: 8317432: Async UL: Use memcpy instead of strcpy in Message ctr
• 126f2ac: 8318006: remove unused net related coding
• 4d90420: 8315026: ProcessHandle implementation listing processes on AIX should use getprocs64
• 1082c0e: 8317677: Specialize Vtablestubs::entry_for() for VtableBlob
• ec310fe: 8317919: pthread_attr_init handle return value and destroy pthread_attr_t object
• ff0b397: 8317959: Check return values of malloc in native java.base coding
• c88b387: 8317987: C2 recompilations cause high memory footprint
• ... and 32 more: https://git.openjdk.org/jdk/compare/731fb4eea21ab67d90970d7c6107fb0a4fbee9ec...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]

/integrate

[openjdk]

Going to push as commit 6273ab9.
Since your change was applied there have been 42 commits pushed to the master branch:

• 7d31146: 8318028: Remove unused class="centered" from FocusCycle.svg
• 45b7aed: 8317433: Async UL: Only grab lock once when write():ing
• 266636d: 8315940: ARM32: Move field resolution information out of the cpCache
• e490cf9: 8317432: Async UL: Use memcpy instead of strcpy in Message ctr
• 126f2ac: 8318006: remove unused net related coding
• 4d90420: 8315026: ProcessHandle implementation listing processes on AIX should use getprocs64
• 1082c0e: 8317677: Specialize Vtablestubs::entry_for() for VtableBlob
• ec310fe: 8317919: pthread_attr_init handle return value and destroy pthread_attr_t object
• ff0b397: 8317959: Check return values of malloc in native java.base coding
• c88b387: 8317987: C2 recompilations cause high memory footprint
• ... and 32 more: https://git.openjdk.org/jdk/compare/731fb4eea21ab67d90970d7c6107fb0a4fbee9ec...master

Your commit was automatically rebased without conflicts.

[openjdk]

@dfuch Pushed as commit 6273ab9.

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