8274687: JDWP deadlocks if some Java thread reaches wait in blockOnDebuggerSuspend #5849

reinrich · 2021-10-07T13:50:49Z

This change fixes deadlocks described in the JBS-bug by:

Releasing handlerLock before waiting on threadLock in blockOnDebuggerSuspend()
Notifying on threadLock in threadControl_reset()

Also the actions in handleAppResumeBreakpoint() are moved/deferred until
doPendingTasks() runs. This is necessary because an event handler must not
release handlerLock first and foremost because handlers are called while
iterating the handler chain for an event type which is protected by handlerLock
(see #5805)

The first commit delays the cleanup actions after leaving the loop in
debugLoop_run(). It allows to reproduce the deadlock running the dispose003
test with the command

make run-test TEST=test/hotspot/jtreg/vmTestbase/nsk/jdi/VirtualMachine/dispose/dispose003

The second commit adds a new test that reproduces the deadlock when calling
threadControl_resumeThread() while a thread is waiting in
blockOnDebuggerSuspend().

The third commit contains the fix described above. With it the deadlocks
cannot be reproduced anymore.

The forth commit removes the diagnostic code introduced with the first commit again.

The fix passed

test/hotspot/jtreg/serviceability/jdwp
test/jdk/com/sun/jdi
test/hotspot/jtreg/vmTestbase/nsk/jdwp
test/hotspot/jtreg/vmTestbase/nsk/jdi

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed

Issue

JDK-8274687: JDWP deadlocks if some Java thread reaches wait in blockOnDebuggerSuspend

Reviewers

Chris Plummer (@plummercj - Reviewer) ⚠️ Review applies to 3383a23
Serguei Spitsyn (@sspitsyn - Reviewer) ⚠️ Review applies to 85dfaef
Ralf Schmelter (@schmelter-sap - Reviewer) ⚠️ Review applies to 3383a23

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/5849/head:pull/5849
$ git checkout pull/5849

Update a local copy of the PR:
$ git checkout pull/5849
$ git pull https://git.openjdk.java.net/jdk pull/5849/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 5849

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/5849.diff

…est dispose003

bridgekeeper · 2021-10-07T13:52:06Z

👋 Welcome back rrich! 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 · 2021-10-07T13:55:22Z

@reinrich The following label will be automatically applied to this pull request:

serviceability

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 · 2021-10-07T14:20:25Z

Webrevs

test/jdk/com/sun/jdi/ResumeAfterThreadResumeCallTest.java

src/jdk.jdwp.agent/share/native/libjdwp/threadControl.c

openjdk · 2021-10-22T17:51:11Z

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

8274687: JDWP deadlocks if some Java thread reaches wait in blockOnDebuggerSuspend

Reviewed-by: cjplummer, sspitsyn, rschmelter

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

55b36c6: 8276825: hotspot/runtime/SelectionResolution test errors
0f23c6a: 8276926: Use String.valueOf() when initializing File.separator and File.pathSeparator
a0b8445: 8276846: JDK-8273416 is incomplete for UseSSE=1
a3f710e: 8276215: Intrinsics matchers should handle native method flags better
0f463a7: 8276845: (fs) java/nio/file/spi/SetDefaultProvider.java fails on x86_32
e01d6d0: 8276679: Malformed Javadoc inline tags in JDK source in javax/swing
fd0a25e: 8276805: java/awt/print/PrinterJob/CheckPrivilege.java fails due to disabled SecurityManager
403f318: 8276854: Windows GHA builds fail due to broken Cygwin
e91e9d8: 8276721: G1: Refine G1EvacFailureObjectsSet::iterate
8822d41: 8274736: Concurrent read/close of SSLSockets causes SSLSessions to be invalidated unnecessarily
... and 431 more: https://git.openjdk.java.net/jdk/compare/29dcbb72a2d9b224203d92ad3224cf149a7d08de...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.

src/jdk.jdwp.agent/share/native/libjdwp/threadControl.c

sspitsyn

Hi Richard,
The fix looks okay to me.
I've inlined one minor suggestion.
Thanks,
Serguei

sspitsyn · 2021-11-10T01:27:40Z

src/jdk.jdwp.agent/share/native/libjdwp/threadControl.c

+         */
+        debugMonitorExit(threadLock);
+        eventHandler_lock();
+        debugMonitorEnter(threadLock);


The lines 2192-2194 look like a hack to me.
But I see you have filed an enhancement to fix this.
I'm okay to address it later.

Thanks for looking at this Serguei.
I agree that this part looks a little odd. It is still correct, though, and the changes are limited to the hardly used code responsible for handling j.l.Thread.resume() calls which is a good thing IMHO.

schmelter-sap

LGTM

src/jdk.jdwp.agent/share/native/libjdwp/threadControl.c

reinrich · 2021-11-15T07:01:11Z

Thanks for the reviews @plummercj, @sspitsyn, @schmelter-sap!

/integrate

openjdk · 2021-11-15T07:02:37Z

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

296780c: 8276983: Small fixes to DumpAllocStat::print_stats
8c5f030: 8276453: Undefined behavior in C1 LIR_OprDesc causes SEGV in fastdebug build
176d21d: 8276824: refactor Thread::is_JavaThread_protected
74f3e69: 8277071: [BACKOUT] JDK-8276743 Make openjdk build Zip Archive generation "reproducible"
b85500e: 8276123: ZipFile::getEntry will not return a file entry when there is a directory entry of the same name within a Zip File
0d2980c: 8258192: Obsolete the CriticalJNINatives flag
5a2452c: 8274835: Remove unnecessary castings in java.base
3b2585c: 8276658: Clean up JNI local handles code
aeba653: 8276743: Make openjdk build Zip Archive generation "reproducible"
51a5731: 8277016: Use blessed modifier order in jdk.httpserver
... and 472 more: https://git.openjdk.java.net/jdk/compare/29dcbb72a2d9b224203d92ad3224cf149a7d08de...master

Your commit was automatically rebased without conflicts.

openjdk · 2021-11-15T07:02:51Z

@reinrich Pushed as commit ca2efb7.

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

reinrich added 2 commits October 7, 2021 10:50

Delay cleanup after JDWP Dispose command to trigger deadlock in the t…

eb889a1

…est dispose003

New test that deadlocks target VM calling ThreadReference.resume().

6f6e8fb

openjdk bot added the serviceability serviceability-dev@openjdk.org label Oct 7, 2021

reinrich added 2 commits October 7, 2021 16:00

Fix that prevents deadlocks.

5591360

Remove delay in cleanup again.

8c1b5ce

reinrich force-pushed the 8274687_jdwp_deadlock_in_blockOnDebuggerSuspend___v2 branch from b59d303 to 8c1b5ce Compare October 7, 2021 14:01

reinrich marked this pull request as ready for review October 7, 2021 14:13

openjdk bot added the rfr Pull request is ready for review label Oct 7, 2021

plummercj reviewed Oct 12, 2021

View reviewed changes

test/jdk/com/sun/jdi/ResumeAfterThreadResumeCallTest.java Outdated Show resolved Hide resolved

reinrich added 3 commits October 12, 2021 09:39

Improve @summary section of test.

6cc006b

Improve @summary section of test.

e198e5e

Improve @summary section of test.

8c51e71

plummercj reviewed Oct 13, 2021

View reviewed changes

src/jdk.jdwp.agent/share/native/libjdwp/threadControl.c Outdated Show resolved Hide resolved

reinrich added 4 commits October 13, 2021 15:37

Changes based on plummercj's comments.

1d241cd

Adding source filename and line numbers to printStack().

806bceb

Better comments on lock usage.

20f1f31

Call blockOnDebuggerSuspend() after setup of the resumer tracking.

0b0fef0

plummercj reviewed Oct 19, 2021

View reviewed changes

reinrich added 2 commits October 20, 2021 09:39

Improve comments.

53d6b9d

Hoist locking from trackAppResume() up to it its caller doPendingTasks()

c884a76

plummercj reviewed Oct 22, 2021

View reviewed changes

src/jdk.jdwp.agent/share/native/libjdwp/threadControl.c Outdated Show resolved Hide resolved

Improve comment as suggested by Chris.

3383a23

plummercj approved these changes Oct 22, 2021

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Oct 22, 2021

sspitsyn reviewed Nov 10, 2021

View reviewed changes

src/jdk.jdwp.agent/share/native/libjdwp/threadControl.c Outdated Show resolved Hide resolved

sspitsyn approved these changes Nov 10, 2021

View reviewed changes

Improve comment as suggested by Serguei.

85dfaef

schmelter-sap approved these changes Nov 10, 2021

View reviewed changes

src/jdk.jdwp.agent/share/native/libjdwp/threadControl.c Outdated Show resolved Hide resolved

src/jdk.jdwp.agent/share/native/libjdwp/threadControl.c Outdated Show resolved Hide resolved

sspitsyn approved these changes Nov 10, 2021

View reviewed changes

Changes based on Ralf's feedback.

1da43f7

openjdk bot closed this Nov 15, 2021

openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Nov 15, 2021

reinrich deleted the 8274687_jdwp_deadlock_in_blockOnDebuggerSuspend___v2 branch October 20, 2022 06:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8274687: JDWP deadlocks if some Java thread reaches wait in blockOnDebuggerSuspend #5849

8274687: JDWP deadlocks if some Java thread reaches wait in blockOnDebuggerSuspend #5849

reinrich commented Oct 7, 2021 •

edited by openjdk bot

Loading

bridgekeeper bot commented Oct 7, 2021

openjdk bot commented Oct 7, 2021

mlbridge bot commented Oct 7, 2021 •

edited

Loading

openjdk bot commented Oct 22, 2021 •

edited

Loading

sspitsyn left a comment

sspitsyn Nov 10, 2021

reinrich Nov 10, 2021

schmelter-sap left a comment

reinrich commented Nov 15, 2021

openjdk bot commented Nov 15, 2021

openjdk bot commented Nov 15, 2021

8274687: JDWP deadlocks if some Java thread reaches wait in blockOnDebuggerSuspend #5849

8274687: JDWP deadlocks if some Java thread reaches wait in blockOnDebuggerSuspend #5849

Conversation

reinrich commented Oct 7, 2021 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Oct 7, 2021

openjdk bot commented Oct 7, 2021

mlbridge bot commented Oct 7, 2021 • edited Loading

Webrevs

openjdk bot commented Oct 22, 2021 • edited Loading

sspitsyn left a comment

Choose a reason for hiding this comment

sspitsyn Nov 10, 2021

Choose a reason for hiding this comment

reinrich Nov 10, 2021

Choose a reason for hiding this comment

schmelter-sap left a comment

Choose a reason for hiding this comment

reinrich commented Nov 15, 2021

openjdk bot commented Nov 15, 2021

openjdk bot commented Nov 15, 2021

reinrich commented Oct 7, 2021 •

edited by openjdk bot

Loading

mlbridge bot commented Oct 7, 2021 •

edited

Loading

openjdk bot commented Oct 22, 2021 •

edited

Loading