Re: RFR: 8274687: JDWP deadlocks if some Java thread reaches wait in blockOnDebuggerSuspend [v10]
On Fri, 22 Oct 2021 17:48:36 GMT, Chris Plummer wrote: >> Richard Reingruber has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Improve comment as suggested by Chris. > > Marked as reviewed by cjplummer (Reviewer). Thanks for the reviews @plummercj, @sspitsyn, @schmelter-sap! - PR: https://git.openjdk.java.net/jdk/pull/5849
Re: RFR: 8274687: JDWP deadlocks if some Java thread reaches wait in blockOnDebuggerSuspend [v12]
> 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 https://github.com/openjdk/jdk/pull/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 Richard Reingruber has updated the pull request incrementally with one additional commit since the last revision: Changes based on Ralf's feedback. - Changes: - all: https://git.openjdk.java.net/jdk/pull/5849/files - new: https://git.openjdk.java.net/jdk/pull/5849/files/85dfaef1..1da43f79 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5849&range=11 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5849&range=10-11 Stats: 28 lines in 1 file changed: 9 ins; 10 del; 9 mod Patch: https://git.openjdk.java.net/jdk/pull/5849.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5849/head:pull/5849 PR: https://git.openjdk.java.net/jdk/pull/5849
Re: RFR: 8274687: JDWP deadlocks if some Java thread reaches wait in blockOnDebuggerSuspend [v10]
On Mon, 8 Nov 2021 19:10:23 GMT, Ralf Schmelter wrote:
>> Richard Reingruber has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Improve comment as suggested by Chris.
>
> src/jdk.jdwp.agent/share/native/libjdwp/threadControl.c line 812:
>
>> 810: {
>> 811: jthread resumer = evinfo->thread;
>> 812: ThreadNode *node;
>
> You could move the declaration into the if() block below, since it is not
> needed elsewhere.
Done.
> src/jdk.jdwp.agent/share/native/libjdwp/threadControl.c line 2192:
>
>> 2190: * ordering handlerLock has to be acquired before threadLock.
>> 2191: */
>> 2192: debugMonitorExit(threadLock);
>
> You could move this to the if (resumer != NULL) block, since otherwise all
> the locking and unlocking is not needed anyways as far as I can see.
Done. Thanks for reviewing!
-
PR: https://git.openjdk.java.net/jdk/pull/5849
Re: RFR: 8274687: JDWP deadlocks if some Java thread reaches wait in blockOnDebuggerSuspend [v11]
On Wed, 10 Nov 2021 09:55:30 GMT, Richard Reingruber wrote: >> 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 https://github.com/openjdk/jdk/pull/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 > > Richard Reingruber has updated the pull request incrementally with one > additional commit since the last revision: > > Improve comment as suggested by Serguei. Marked as reviewed by sspitsyn (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/5849
Re: RFR: 8274687: JDWP deadlocks if some Java thread reaches wait in blockOnDebuggerSuspend [v10]
On Fri, 22 Oct 2021 06:21:37 GMT, Richard Reingruber wrote:
>> 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 https://github.com/openjdk/jdk/pull/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
>
> Richard Reingruber has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Improve comment as suggested by Chris.
LGTM
src/jdk.jdwp.agent/share/native/libjdwp/threadControl.c line 812:
> 810: {
> 811: jthread resumer = evinfo->thread;
> 812: ThreadNode *node;
You could move the declaration into the if() block below, since it is not
needed elsewhere.
src/jdk.jdwp.agent/share/native/libjdwp/threadControl.c line 2192:
> 2190: * ordering handlerLock has to be acquired before threadLock.
> 2191: */
> 2192: debugMonitorExit(threadLock);
You could move this to the if (resumer != NULL) block, since otherwise all the
locking and unlocking is not needed anyways as far as I can see.
-
Marked as reviewed by rschmelter (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/5849
Re: RFR: 8274687: JDWP deadlocks if some Java thread reaches wait in blockOnDebuggerSuspend [v10]
On Wed, 10 Nov 2021 01:24:58 GMT, Serguei Spitsyn wrote: >> Richard Reingruber has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Improve comment as suggested by Chris. > > src/jdk.jdwp.agent/share/native/libjdwp/threadControl.c line 2189: > >> 2187: >> 2188: /* >> 2189: * trackAppResume() (indirectly) aquires handlerLock. For >> proper lock > > I'd suggest to get rid of '()' brackets in this comment line, so it will be: >`* trackAppResume indirectly aquires handlerLock. For proper lock` Sure. Done. - PR: https://git.openjdk.java.net/jdk/pull/5849
Re: RFR: 8274687: JDWP deadlocks if some Java thread reaches wait in blockOnDebuggerSuspend [v11]
> 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 https://github.com/openjdk/jdk/pull/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 Richard Reingruber has updated the pull request incrementally with one additional commit since the last revision: Improve comment as suggested by Serguei. - Changes: - all: https://git.openjdk.java.net/jdk/pull/5849/files - new: https://git.openjdk.java.net/jdk/pull/5849/files/3383a236..85dfaef1 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5849&range=10 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5849&range=09-10 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/5849.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5849/head:pull/5849 PR: https://git.openjdk.java.net/jdk/pull/5849
Re: RFR: 8274687: JDWP deadlocks if some Java thread reaches wait in blockOnDebuggerSuspend [v10]
On Wed, 10 Nov 2021 01:27:40 GMT, Serguei Spitsyn wrote: >> Richard Reingruber has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Improve comment as suggested by Chris. > > src/jdk.jdwp.agent/share/native/libjdwp/threadControl.c line 2194: > >> 2192: debugMonitorExit(threadLock); >> 2193: eventHandler_lock(); >> 2194: 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. - PR: https://git.openjdk.java.net/jdk/pull/5849
Re: RFR: 8274687: JDWP deadlocks if some Java thread reaches wait in blockOnDebuggerSuspend [v10]
On Fri, 22 Oct 2021 06:21:37 GMT, Richard Reingruber wrote: >> 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 https://github.com/openjdk/jdk/pull/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 > > Richard Reingruber has updated the pull request incrementally with one > additional commit since the last revision: > > Improve comment as suggested by Chris. Hi Richard, The fix looks okay to me. I've inlined one minor suggestion. Thanks, Serguei src/jdk.jdwp.agent/share/native/libjdwp/threadControl.c line 2194: > 2192: debugMonitorExit(threadLock); > 2193: eventHandler_lock(); > 2194: 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. - Marked as reviewed by sspitsyn (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/5849
Re: RFR: 8274687: JDWP deadlocks if some Java thread reaches wait in blockOnDebuggerSuspend [v10]
On Fri, 22 Oct 2021 06:21:37 GMT, Richard Reingruber wrote: >> 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 https://github.com/openjdk/jdk/pull/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 > > Richard Reingruber has updated the pull request incrementally with one > additional commit since the last revision: > > Improve comment as suggested by Chris. src/jdk.jdwp.agent/share/native/libjdwp/threadControl.c line 2189: > 2187: > 2188: /* > 2189: * trackAppResume() (indirectly) aquires handlerLock. For > proper lock I'd suggest to get rid of '()' brackets in this comment line, so it will be: `* trackAppResume indirectly aquires handlerLock. For proper lock` - PR: https://git.openjdk.java.net/jdk/pull/5849
Re: RFR: 8274687: JDWP deadlocks if some Java thread reaches wait in blockOnDebuggerSuspend [v10]
On Fri, 22 Oct 2021 06:21:37 GMT, Richard Reingruber wrote: >> 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 https://github.com/openjdk/jdk/pull/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 > > Richard Reingruber has updated the pull request incrementally with one > additional commit since the last revision: > > Improve comment as suggested by Chris. Marked as reviewed by cjplummer (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/5849
Re: RFR: 8274687: JDWP deadlocks if some Java thread reaches wait in blockOnDebuggerSuspend [v9]
On Fri, 22 Oct 2021 04:02:04 GMT, Chris Plummer wrote: >> Richard Reingruber has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Hoist locking from trackAppResume() up to it its caller doPendingTasks() > > src/jdk.jdwp.agent/share/native/libjdwp/threadControl.c line 2212: > >> 2210: * handlerLock is not needed anymore. We release it before >> calling >> 2211: * blockOnDebuggerSuspend() because it is required for resumes >> by the >> 2212: * debugger so we cannot wait for that holding handlerLock. > > * handlerLock is not needed anymore. We must release it before calling > * blockOnDebuggerSuspend() because it is required for resumes done by > * the debugger. If resumee is currently suspended by the debugger, > then > * blockOnDebuggerSuspend() will block until a debugger resume is > done. > * If it blocks while holding the handlerLock, then the resume will > deadlock. Done. - PR: https://git.openjdk.java.net/jdk/pull/5849
Re: RFR: 8274687: JDWP deadlocks if some Java thread reaches wait in blockOnDebuggerSuspend [v10]
> 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 https://github.com/openjdk/jdk/pull/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 Richard Reingruber has updated the pull request incrementally with one additional commit since the last revision: Improve comment as suggested by Chris. - Changes: - all: https://git.openjdk.java.net/jdk/pull/5849/files - new: https://git.openjdk.java.net/jdk/pull/5849/files/c884a76e..3383a236 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5849&range=09 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5849&range=08-09 Stats: 5 lines in 1 file changed: 2 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/5849.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5849/head:pull/5849 PR: https://git.openjdk.java.net/jdk/pull/5849
Re: RFR: 8274687: JDWP deadlocks if some Java thread reaches wait in blockOnDebuggerSuspend [v7]
On Fri, 22 Oct 2021 04:04:13 GMT, Chris Plummer wrote: >> I have hoisted the locking from `trackAppResume()` up to `doPendingTasks()` >> where it is more visible. The change in that form is sufficient to fix the >> deadlock issues. Would it be ok for you to do further enhancements in a >> follow-up RFE? > > Yes, an RFE is fine. The changes look good. I just added one suggestion to > improve a comment. Thanks. I've created https://bugs.openjdk.java.net/browse/JDK-8275764. - PR: https://git.openjdk.java.net/jdk/pull/5849
Re: RFR: 8274687: JDWP deadlocks if some Java thread reaches wait in blockOnDebuggerSuspend [v7]
On Thu, 21 Oct 2021 19:38:41 GMT, Richard Reingruber wrote: >> If you are worried about another thread changing those fields after they >> have already been checked, then you can use the threadLock around them also. >> So you can grab threadLock before the `if (node->handlingAppResume)` and >> release after the `pendingInterrupt` and `pendingStop` references. I don't >> think it's really needed, because this is the only place where the flags are >> set false (and some action is taken when true), but there is no harm in >> holding the threadLock here, and it doesn't make the locking any more >> complicated. > > I have hoisted the locking from `trackAppResume()` up to `doPendingTasks()` > where it is more visible. The change in that form is sufficient to fix the > deadlock issues. Would it be ok for you to do further enhancements in a > follow-up RFE? Yes, an RFE is fine. The changes look good. I just added one suggestion to improve a comment. - PR: https://git.openjdk.java.net/jdk/pull/5849
Re: RFR: 8274687: JDWP deadlocks if some Java thread reaches wait in blockOnDebuggerSuspend [v9]
On Thu, 21 Oct 2021 19:37:41 GMT, Richard Reingruber wrote: >> 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 https://github.com/openjdk/jdk/pull/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 > > Richard Reingruber has updated the pull request incrementally with one > additional commit since the last revision: > > Hoist locking from trackAppResume() up to it its caller doPendingTasks() src/jdk.jdwp.agent/share/native/libjdwp/threadControl.c line 2212: > 2210: * handlerLock is not needed anymore. We release it before > calling > 2211: * blockOnDebuggerSuspend() because it is required for resumes > by the > 2212: * debugger so we cannot wait for that holding handlerLock. * handlerLock is not needed anymore. We must release it before calling * blockOnDebuggerSuspend() because it is required for resumes done by * the debugger. If resumee is currently suspended by the debugger, then * blockOnDebuggerSuspend() will block until a debugger resume is done. * If it blocks while holding the handlerLock, then the resume will deadlock. - PR: https://git.openjdk.java.net/jdk/pull/5849
Re: RFR: 8274687: JDWP deadlocks if some Java thread reaches wait in blockOnDebuggerSuspend [v7]
On Thu, 21 Oct 2021 15:05:19 GMT, Chris Plummer wrote: >>> move the locking and unlocking into doPendingTasks(). At the start of the >>> node->handlingAppResume block, grab handlerLock and threadLock, and explain >>> that handlerLock is being grabbed because trackAppResume() will (indirectly) >>> try to grab it, and grabbing it before threadLock is necessary for lock >>> ordering. Then grab threadLock and hold onto it until the end of the block. >> >> So threadLock would be released at the end of the new block that depends on >> node->handlingAppResume. After that block follow accesses to >> `pendingInterrupt` >> and `pendingStop` that would be unsynchronized then. I think they do need to >> be >> synchronized through threadLock though. > > If you are worried about another thread changing those fields after they have > already been checked, then you can use the threadLock around them also. So > you can grab threadLock before the `if (node->handlingAppResume)` and release > after the `pendingInterrupt` and `pendingStop` references. I don't think it's > really needed, because this is the only place where the flags are set false > (and some action is taken when true), but there is no harm in holding the > threadLock here, and it doesn't make the locking any more complicated. I have hoisted the locking from `trackAppResume()` up to `doPendingTasks()` where it is more visible. The change in that form is sufficient to fix the deadlock issues. Would it be ok for you to do further enhancements in a follow-up RFE? - PR: https://git.openjdk.java.net/jdk/pull/5849
Re: RFR: 8274687: JDWP deadlocks if some Java thread reaches wait in blockOnDebuggerSuspend [v9]
> 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 https://github.com/openjdk/jdk/pull/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 Richard Reingruber has updated the pull request incrementally with one additional commit since the last revision: Hoist locking from trackAppResume() up to it its caller doPendingTasks() - Changes: - all: https://git.openjdk.java.net/jdk/pull/5849/files - new: https://git.openjdk.java.net/jdk/pull/5849/files/53d6b9de..c884a76e Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5849&range=08 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5849&range=07-08 Stats: 28 lines in 1 file changed: 12 ins; 12 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/5849.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5849/head:pull/5849 PR: https://git.openjdk.java.net/jdk/pull/5849
Re: RFR: 8274687: JDWP deadlocks if some Java thread reaches wait in blockOnDebuggerSuspend [v7]
On Thu, 21 Oct 2021 08:01:30 GMT, Richard Reingruber wrote: >> BTW, normally I would not suggest less locking simply because I *thought* it >> might not be needed. However, you already temporarily release and then >> regrab the lock. This means there is a period of time where the locking is >> not needed. What I've suggested is to better define (and expand) that period >> of time so the locking can be made cleaner. I think the actual expansion of >> time where the lock would no longer be held is pretty small, with a well >> defined set of code being executed that to me does not appear to require the >> lock, and if it did require the lock, then probably releasing the lock in >> the first place is a bug. > >> move the locking and unlocking into doPendingTasks(). At the start of the >> node->handlingAppResume block, grab handlerLock and threadLock, and explain >> that handlerLock is being grabbed because trackAppResume() will (indirectly) >> try to grab it, and grabbing it before threadLock is necessary for lock >> ordering. Then grab threadLock and hold onto it until the end of the block. > > So threadLock would be released at the end of the new block that depends on > node->handlingAppResume. After that block follow accesses to > `pendingInterrupt` > and `pendingStop` that would be unsynchronized then. I think they do need to > be > synchronized through threadLock though. If you are worried about another thread changing those fields after they have already been checked, then you can use the threadLock around them also. So you can grab threadLock before the `if (node->handlingAppResume)` and release after the `pendingInterrupt` and `pendingStop` references. I don't think it's really needed, because this is the only place where the flags are set false (and some action is taken when true), but there is no harm in holding the threadLock here, and it doesn't make the locking any more complicated. - PR: https://git.openjdk.java.net/jdk/pull/5849
Re: RFR: 8274687: JDWP deadlocks if some Java thread reaches wait in blockOnDebuggerSuspend [v7]
On Thu, 21 Oct 2021 03:34:30 GMT, Chris Plummer wrote:
>> You already release threadLock temporarily in `trackAppResume()` and you
>> used to release it temporarily in doPendingTasks(). I'm suggesting to
>> instead release it in `threadControl_onEventHandlerExit()` before calling
>> `doPendingTasks()` and then grab it again (after first grabbing the
>> handlerLock) in `doPendingTasks()` before calling `trackAppResume()`. What
>> can be modified in the ThreadNode during that time that you are concerned
>> about? threadLock would still be held when `blockOnDebuggerSuspend()` is
>> called. The only interesting lines that are currently executed while
>> holding the threadLock but no longer would be are:
>>
>> 2193 if (node->handlingAppResume) {
>> 2194 jthread resumer = node->thread;
>> 2195 jthread resumee = getResumee(resumer);
>>
>> I don't think any of those can change, because only the currently executing
>> thread can change them (when the Thread.resume() breakpoint is hit).
>>
>> The suspendCount is only meaningful when in `blockOnDebuggerSuspend()`, and
>> we'll be under the threadLock by that point.
>
> BTW, normally I would not suggest less locking simply because I *thought* it
> might not be needed. However, you already temporarily release and then regrab
> the lock. This means there is a period of time where the locking is not
> needed. What I've suggested is to better define (and expand) that period of
> time so the locking can be made cleaner. I think the actual expansion of time
> where the lock would no longer be held is pretty small, with a well defined
> set of code being executed that to me does not appear to require the lock,
> and if it did require the lock, then probably releasing the lock in the first
> place is a bug.
> move the locking and unlocking into doPendingTasks(). At the start of the
> node->handlingAppResume block, grab handlerLock and threadLock, and explain
> that handlerLock is being grabbed because trackAppResume() will (indirectly)
> try to grab it, and grabbing it before threadLock is necessary for lock
> ordering. Then grab threadLock and hold onto it until the end of the block.
So threadLock would be released at the end of the new block that depends on
node->handlingAppResume. After that block follow accesses to `pendingInterrupt`
and `pendingStop` that would be unsynchronized then. I think they do need to be
synchronized through threadLock though.
-
PR: https://git.openjdk.java.net/jdk/pull/5849
Re: RFR: 8274687: JDWP deadlocks if some Java thread reaches wait in blockOnDebuggerSuspend [v7]
On Thu, 21 Oct 2021 03:24:40 GMT, Chris Plummer wrote:
>> But fields of ThreadNode "node" (aka "resumer") are read and also modified
>> in `doPendingTasks()` and also in `threadControl_onEventHandlerExit()`. IMHO
>> this needs to be synchronized with threadLock. At least it is not obvious
>> that the synchronization is not needed.
>
> You already release threadLock temporarily in `trackAppResume()` and you used
> to release it temporarily in doPendingTasks(). I'm suggesting to instead
> release it in `threadControl_onEventHandlerExit()` before calling
> `doPendingTasks()` and then grab it again (after first grabbing the
> handlerLock) in `doPendingTasks()` before calling `trackAppResume()`. What
> can be modified in the ThreadNode during that time that you are concerned
> about? threadLock would still be held when `blockOnDebuggerSuspend()` is
> called. The only interesting lines that are currently executed while holding
> the threadLock but no longer would be are:
>
> 2193 if (node->handlingAppResume) {
> 2194 jthread resumer = node->thread;
> 2195 jthread resumee = getResumee(resumer);
>
> I don't think any of those can change, because only the currently executing
> thread can change them (when the Thread.resume() breakpoint is hit).
>
> The suspendCount is only meaningful when in `blockOnDebuggerSuspend()`, and
> we'll be under the threadLock by that point.
BTW, normally I would not suggest less locking simply because I *thought* it
might not be needed. However, you already temporarily release and then regrab
the lock. This means there is a period of time where the locking is not needed.
What I've suggested is to better define (and expand) that period of time so the
locking can be made cleaner. I think the actual expansion of time where the
lock would no longer be held is pretty small, with a well defined set of code
being executed that to me does not appear to require the lock, and if it did
require the lock, then probably releasing the lock in the first place is a bug.
-
PR: https://git.openjdk.java.net/jdk/pull/5849
Re: RFR: 8274687: JDWP deadlocks if some Java thread reaches wait in blockOnDebuggerSuspend [v7]
On Wed, 20 Oct 2021 06:50:51 GMT, Richard Reingruber wrote:
>> src/jdk.jdwp.agent/share/native/libjdwp/threadControl.c line 770:
>>
>>> 768: * handlerLock which has to be acquired before threadLock.
>>> 769: */
>>> 770: debugMonitorExit(threadLock);
>>
>> I think we can avoid the exit here. threadLock was grabbed in
>> `threadControl_onEventHandlerExit()`. It probably should be released before
>> calling `doPendingTasks()`. My suggestion would be to first release it right
>> after the `findThread()` call, and then in the `ei == EI_THREAD_END` block
>> grab it again at the start of the block and release at the end of the block.
>
> But fields of ThreadNode "node" (aka "resumer") are read and also modified in
> `doPendingTasks()` and also in `threadControl_onEventHandlerExit()`. IMHO
> this needs to be synchronized with threadLock. At least it is not obvious
> that the synchronization is not needed.
You already release threadLock temporarily in `trackAppResume()` and you used
to release it temporarily in doPendingTasks(). I'm suggesting to instead
release it in `threadControl_onEventHandlerExit()` before calling
`doPendingTasks()` and then grab it again (after first grabbing the
handlerLock) in `doPendingTasks()` before calling `trackAppResume()`. What can
be modified in the ThreadNode during that time that you are concerned about?
threadLock would still be held when `blockOnDebuggerSuspend()` is called. The
only interesting lines that are currently executed while holding the threadLock
but no longer would be are:
2193 if (node->handlingAppResume) {
2194 jthread resumer = node->thread;
2195 jthread resumee = getResumee(resumer);
I don't think any of those can change, because only the currently executing
thread can change them (when the Thread.resume() breakpoint is hit).
The suspendCount is only meaningful when in `blockOnDebuggerSuspend()`, and
we'll be under the threadLock by that point.
-
PR: https://git.openjdk.java.net/jdk/pull/5849
Re: RFR: 8274687: JDWP deadlocks if some Java thread reaches wait in blockOnDebuggerSuspend [v8]
> 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 https://github.com/openjdk/jdk/pull/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 Richard Reingruber has updated the pull request incrementally with one additional commit since the last revision: Improve comments. - Changes: - all: https://git.openjdk.java.net/jdk/pull/5849/files - new: https://git.openjdk.java.net/jdk/pull/5849/files/0b0fef0e..53d6b9de Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5849&range=07 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5849&range=06-07 Stats: 7 lines in 1 file changed: 4 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/5849.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5849/head:pull/5849 PR: https://git.openjdk.java.net/jdk/pull/5849
Re: RFR: 8274687: JDWP deadlocks if some Java thread reaches wait in blockOnDebuggerSuspend [v7]
On Tue, 19 Oct 2021 20:21:59 GMT, Chris Plummer wrote: >> Richard Reingruber has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Call blockOnDebuggerSuspend() after setup of the resumer tracking. > > src/jdk.jdwp.agent/share/native/libjdwp/threadControl.c line 772: > >> 770: debugMonitorExit(threadLock); >> 771: eventHandler_lock(); /* for proper lock order */ >> 772: debugMonitorEnter(threadLock); > > Somewhere we need to mention that `trackAppResume()` exits with threadLock > still being held, although with my recommended changes below this would no > longer be the case. If we cannot release `threadLock` before calling `doPendingTasks()` then I would suggest to comment that the caller of `trackAppResume()` is expected to hold `threadLock` and that it will be temporarily released. > src/jdk.jdwp.agent/share/native/libjdwp/threadControl.c line 807: > >> 805: } >> 806: >> 807: eventHandler_unlock(); > > This is still a bit odd looking because we grabbed this lock for lock > ordering purposes, but are now releasing it out of order. But it's starting > to sink in with me that the root of our problem here is that lock order > dictates grabbing handlerLock first, and we need to eventually wait on > threadLock but with handlerLock released, which suggest the lock ordering is > reversed. There is probably some design bug here that results in that, but > I'd hate to mess with the ordering of these two locks to try to fix it. Maybe > a 3rd lock would help (wait on some new lock instead of threadLock). We could > grab that lock first in `doPendingTasks()`, and not have to exit > `trackAppResume()` with threadLock held, but again this might be more risk > than it is worth. > > So it we aren't going to change the lock ordering or introduce a 3rd lock, > then my suggestion would be (after making the above suggested change to > release threadLock before calling `threadControl_onEventHandlerExit()`) to > move the locking and unlocking into `doPendingTasks()`. At the start of the > `node->handlingAppResume` block, grab handlerLock and threadLock, and explain > that handlerLock is being grabbed because trackAppResume() will (indirectly) > try to grab it, and grabbing it before threadLock is necessary for lock > ordering. Then grab threadLock and hold onto it until the end of the block. > Between the `trackAppResume()` and `blockOnDebuggerSuspend()` calls, release > handlerLock and explain that it will not be needed anymore, and has to be > released before calling `blockOnDebuggerSuspend()`. To me it does not look odd. I'd think it is ok to release locks out of order. IMHO adding a new third lock to be waited on in `doPendingTasks()` would make things even more complicated. Accesses to `suspendCount` are synchronized through `threadLock`. Waiting on that lock for change of `suspendCount` is straightforward really. - PR: https://git.openjdk.java.net/jdk/pull/5849
Re: RFR: 8274687: JDWP deadlocks if some Java thread reaches wait in blockOnDebuggerSuspend [v7]
On Tue, 19 Oct 2021 20:17:59 GMT, Chris Plummer wrote: >> Richard Reingruber has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Call blockOnDebuggerSuspend() after setup of the resumer tracking. > > src/jdk.jdwp.agent/share/native/libjdwp/threadControl.c line 770: > >> 768: * handlerLock which has to be acquired before threadLock. >> 769: */ >> 770: debugMonitorExit(threadLock); > > I think we can avoid the exit here. threadLock was grabbed in > `threadControl_onEventHandlerExit()`. It probably should be released before > calling `doPendingTasks()`. My suggestion would be to first release it right > after the `findThread()` call, and then in the `ei == EI_THREAD_END` block > grab it again at the start of the block and release at the end of the block. But fields of ThreadNode "node" (aka "resumer") are read and also modified in `doPendingTasks()` and also in `threadControl_onEventHandlerExit()`. IMHO this needs to be synchronized with threadLock. At least it is not obvious that the synchronization is not needed. > src/jdk.jdwp.agent/share/native/libjdwp/threadControl.c line 771: > >> 769: */ >> 770: debugMonitorExit(threadLock); >> 771: eventHandler_lock(); /* for proper lock order */ > > "for proper lock order during eventHandler_createInternalThreadOnly() calls" Done. - PR: https://git.openjdk.java.net/jdk/pull/5849
Re: RFR: 8274687: JDWP deadlocks if some Java thread reaches wait in blockOnDebuggerSuspend [v7]
On Mon, 18 Oct 2021 20:02:21 GMT, Richard Reingruber wrote: >> 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 https://github.com/openjdk/jdk/pull/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 > > Richard Reingruber has updated the pull request incrementally with one > additional commit since the last revision: > > Call blockOnDebuggerSuspend() after setup of the resumer tracking. src/jdk.jdwp.agent/share/native/libjdwp/threadControl.c line 770: > 768: * handlerLock which has to be acquired before threadLock. > 769: */ > 770: debugMonitorExit(threadLock); I think we can avoid the exit here. threadLock was grabbed in `threadControl_onEventHandlerExit()`. It probably should be released before calling `doPendingTasks()`. My suggestion would be to first release it right after the `findThread()` call, and then in the `ei == EI_THREAD_END` block grab it again at the start of the block and release at the end of the block. src/jdk.jdwp.agent/share/native/libjdwp/threadControl.c line 771: > 769: */ > 770: debugMonitorExit(threadLock); > 771: eventHandler_lock(); /* for proper lock order */ "for proper lock order during eventHandler_createInternalThreadOnly() calls" src/jdk.jdwp.agent/share/native/libjdwp/threadControl.c line 772: > 770: debugMonitorExit(threadLock); > 771: eventHandler_lock(); /* for proper lock order */ > 772: debugMonitorEnter(threadLock); Somewhere we need to mention that `trackAppResume()` exits with threadLock still being held, although with my recommended changes below this would no longer be the case. src/jdk.jdwp.agent/share/native/libjdwp/threadControl.c line 807: > 805: } > 806: > 807: eventHandler_unlock(); This is still a bit odd looking because we grabbed this lock for lock ordering purposes, but are now releasing it out of order. But it's starting to sink in with me that the root of our problem here is that lock order dictates grabbing handlerLock first, and we need to eventually wait on threadLock but with handlerLock released, which suggest the lock ordering is reversed. There is probably some design bug here that results in that, but I'd hate to mess with the ordering of these two locks to try to fix it. Maybe a 3rd lock would help (wait on some new lock instead of threadLock). We could grab that lock first in `doPendingTasks()`, and not have to exit `trackAppResume()` with threadLock held, but again this might be more risk than it is worth. So it we aren't going to change the lock ordering or introduce a 3rd lock, then my suggestion would be (after making the above suggested change to release threadLock before calling `threadControl_onEventHandlerExit()`) to move the locking and unlocking into `doPendingTasks()`. At the start of the `node->handlingAppResume` block, grab handlerLock and threadLock, and explain that handlerLock is being grabbed because trackAppResume() will (indirectly) try to grab it, and grabbing it before threadLock is necessary for lock ordering. Then grab threadLock and hold onto it until the end of the block. Between the `trackAppResume()` and `blockOnDebuggerSuspend()` calls, release handlerLock and explain that it will not be needed anymore, and has to be released before calling `blockOnDebuggerSuspend()`. - PR: https://git.openjdk.java.net/jdk/pull/5849
Re: RFR: 8274687: JDWP deadlocks if some Java thread reaches wait in blockOnDebuggerSuspend [v4]
On Fri, 15 Oct 2021 11:52:09 GMT, Richard Reingruber wrote: > > > Hm, I think this can be simplified by swaping blockOnDebuggerSuspend() and > trackAppResume(). Can't try it today but will on Monday. I've done that with commit https://github.com/openjdk/jdk/pull/5849/commits/0b0fef0e6670c20a0e1e34323847c5a622878469. The locking is clearer I would say. Since `resumeFrameDepth` is now set before resumee's suspendCount is 0 we must block debugger suspends only if `!handlingAppResume` because then we know resumee's suspendCount actually is 0. - PR: https://git.openjdk.java.net/jdk/pull/5849
Re: RFR: 8274687: JDWP deadlocks if some Java thread reaches wait in blockOnDebuggerSuspend [v7]
> 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 https://github.com/openjdk/jdk/pull/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 Richard Reingruber has updated the pull request incrementally with one additional commit since the last revision: Call blockOnDebuggerSuspend() after setup of the resumer tracking. - Changes: - all: https://git.openjdk.java.net/jdk/pull/5849/files - new: https://git.openjdk.java.net/jdk/pull/5849/files/20f1f31b..0b0fef0e Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5849&range=06 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5849&range=05-06 Stats: 95 lines in 2 files changed: 31 ins; 36 del; 28 mod Patch: https://git.openjdk.java.net/jdk/pull/5849.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5849/head:pull/5849 PR: https://git.openjdk.java.net/jdk/pull/5849
Re: RFR: 8274687: JDWP deadlocks if some Java thread reaches wait in blockOnDebuggerSuspend [v4]
On Fri, 15 Oct 2021 09:17:12 GMT, Richard Reingruber wrote: >> Ok. So you just need to reacquire the threadLock before the `findThread()` >> call and before exiting the while loop, and hold it until after the >> `trackAppResume()` call. I guess it ok then. But this exiting of the >> handlerLock here and in `blockOnDebuggerSuspend()` is always going to arouse >> suspicion for anyone that reads the code. The first question will be if the >> lock does not need to be held here, why grab it in the first place? We know >> the answer is this lock ordering issue that turns up when `trackAppResume()` >> is later called, but this will be far from obvious to the reader. It might >> be possible to make this a bit clearer with some code restructuring, like >> maybe combining `blockOnDebuggerSuspend()` and `trackAppResume()` into one >> function (I'm not sure that this will actually help, but maybe something to >> consider), but at the very least we need some comments calling out why >> handlerLock is held in the first place and why it is ok to release it. > > I've added the comments. Maybe I should really combine > `blockOnDebuggerSuspend()` and `trackAppResume()` into one function... Hm, I think this can be simplified by swaping blockOnDebuggerSuspend() and trackAppResume(). Can't try it today but will on Monday. - PR: https://git.openjdk.java.net/jdk/pull/5849
Re: RFR: 8274687: JDWP deadlocks if some Java thread reaches wait in blockOnDebuggerSuspend [v4]
On Thu, 14 Oct 2021 18:33:51 GMT, Chris Plummer wrote: >>> Ok, so you need to hold threadLock from the time `blockOnDebuggerSuspend()` >>> is done waiting on it until after `trackAppResume()` is done, and since >>> `trackAppResume()` needs to grab the handlerLock, and you need to grab the >>> handerLock before the threadLock, you need to jump through some lock >>> grabbing/release hoops. >> >> That's correct. >> >>> However, you are in fact releasing the threadLock for a short time in >>> `blockOnDebuggerSuspend()` after the wait completes. Doesn't this expose >>> the resumee to potential suspending after the wait has completed and before >>> `trackAppResume()` has been called? >> >> That's correct too. I wouldn't see an issue with it though. I think this is >> even a preexisting condition. The current thread can lose the race grabbing >> threadLock after it was notified to the debugger trying to suspend again (if >> there wasn't the deadlock of course) and consequently suspendCount can >> (again) be greater than 0 right after the wait. In that case we simply retry. > > Ok. So you just need to reacquire the threadLock before the `findThread()` > call and before exiting the while loop, and hold it until after the > `trackAppResume()` call. I guess it ok then. But this exiting of the > handlerLock here and in `blockOnDebuggerSuspend()` is always going to arouse > suspicion for anyone that reads the code. The first question will be if the > lock does not need to be held here, why grab it in the first place? We know > the answer is this lock ordering issue that turns up when `trackAppResume()` > is later called, but this will be far from obvious to the reader. It might be > possible to make this a bit clearer with some code restructuring, like maybe > combining `blockOnDebuggerSuspend()` and `trackAppResume()` into one function > (I'm not sure that this will actually help, but maybe something to consider), > but at the very least we need some comments calling out why handlerLock is > held in the first place and why it is ok to release it. I've added the comments. Maybe I should really combine `blockOnDebuggerSuspend()` and `trackAppResume()` into one function... - PR: https://git.openjdk.java.net/jdk/pull/5849
Re: RFR: 8274687: JDWP deadlocks if some Java thread reaches wait in blockOnDebuggerSuspend [v6]
> 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 https://github.com/openjdk/jdk/pull/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 Richard Reingruber has updated the pull request incrementally with one additional commit since the last revision: Better comments on lock usage. - Changes: - all: https://git.openjdk.java.net/jdk/pull/5849/files - new: https://git.openjdk.java.net/jdk/pull/5849/files/806bceb8..20f1f31b Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5849&range=05 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5849&range=04-05 Stats: 26 lines in 1 file changed: 24 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/5849.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5849/head:pull/5849 PR: https://git.openjdk.java.net/jdk/pull/5849
Re: RFR: 8274687: JDWP deadlocks if some Java thread reaches wait in blockOnDebuggerSuspend [v4]
On Thu, 14 Oct 2021 07:44:36 GMT, Richard Reingruber wrote: >> Ok, so you need to hold threadLock from the time `blockOnDebuggerSuspend()` >> is done waiting on it until after `trackAppResume()` is done, and since >> `trackAppResume()` needs to grab the handlerLock, and you need to grab the >> handerLock before the threadLock, you need to jump through some lock >> grabbing/release hoops. However, you are in fact releasing the threadLock >> for a short time in `blockOnDebuggerSuspend()` after the wait completes. >> Doesn't this expose the resumee to potential suspending after the wait has >> completed and before `trackAppResume()` has been called? > >> Ok, so you need to hold threadLock from the time `blockOnDebuggerSuspend()` >> is done waiting on it until after `trackAppResume()` is done, and since >> `trackAppResume()` needs to grab the handlerLock, and you need to grab the >> handerLock before the threadLock, you need to jump through some lock >> grabbing/release hoops. > > That's correct. > >> However, you are in fact releasing the threadLock for a short time in >> `blockOnDebuggerSuspend()` after the wait completes. Doesn't this expose the >> resumee to potential suspending after the wait has completed and before >> `trackAppResume()` has been called? > > That's correct too. I wouldn't see an issue with it though. I think this is > even a preexisting condition. The current thread can lose the race grabbing > threadLock after it was notified to the debugger trying to suspend again (if > there wasn't the deadlock of course) and consequently suspendCount can > (again) be greater than 0 right after the wait. In that case we simply retry. Ok. So you just need to reacquire the threadLock before the `findThread()` call and before exiting the while loop, and hold it until after the `trackAppResume()` call. I guess it ok then. But this exiting of the handlerLock here and in `blockOnDebuggerSuspend()` is always going to arouse suspicion for anyone that reads the code. The first question will be if the lock does not need to be held here, why grab it in the first place? We know the answer is this lock ordering issue that turns up when `trackAppResume()` is later called, but this will be far from obvious to the reader. It might be possible to make this a bit clearer with some code restructuring, like maybe combining `blockOnDebuggerSuspend()` and `trackAppResume()` into one function (I'm not sure that this will actually help, but maybe something to consider), but at the very least we need some comments calling out why handlerLock is held in the first place and why it is ok to release it. - PR: https://git.openjdk.java.net/jdk/pull/5849
Re: RFR: 8274687: JDWP deadlocks if some Java thread reaches wait in blockOnDebuggerSuspend [v4]
On Wed, 13 Oct 2021 20:59:44 GMT, Chris Plummer wrote: > Ok, so you need to hold threadLock from the time `blockOnDebuggerSuspend()` > is done waiting on it until after `trackAppResume()` is done, and since > `trackAppResume()` needs to grab the handlerLock, and you need to grab the > handerLock before the threadLock, you need to jump through some lock > grabbing/release hoops. That's correct. > However, you are in fact releasing the threadLock for a short time in > `blockOnDebuggerSuspend()` after the wait completes. Doesn't this expose the > resumee to potential suspending after the wait has completed and before > `trackAppResume()` has been called? That's correct too. I wouldn't see an issue with it though. I think this is even a preexisting condition. The current thread can lose the race grabbing threadLock after it was notified to the debugger trying to suspend again (if there wasn't the deadlock of course) and consequently suspendCount can (again) be greater than 0 right after the wait. In that case we simply retry. - PR: https://git.openjdk.java.net/jdk/pull/5849
Re: RFR: 8274687: JDWP deadlocks if some Java thread reaches wait in blockOnDebuggerSuspend [v4]
On Wed, 13 Oct 2021 13:24:06 GMT, Richard Reingruber wrote: >> src/jdk.jdwp.agent/share/native/libjdwp/threadControl.c line 2220: >> >>> 2218: * suspends a thread it will remain suspended. >>> 2219: */ >>> 2220: trackAppResume(resumer); >> >> `trackAppResume()` doesn't really need the handlerLock. However, it will >> grab it when calling `eventHandler_createInternalThreadOnly()`. Since you >> want to make sure it is grabbed before threadLock in order to preserve lock >> ordering, that complicates things if we decided not to grab the handlerLock >> in the code above. What I'm now thinking is all that is really needed is to >> hold the threadLock around the call to `blockOnDebuggerSuspend()`, or better >> yet just grab it from within `blockOnDebuggerSuspend()`. You probably don't >> need to do anything with handlerLock or threadLock inside of >> `doPendingTasks()`. > > After returning from `blockOnDebuggerSuspend()` we have to make sure resumee > cannot be suspended by JDWP means otherwise the current thread which is about > to > execute j.l.Thread.resume() will interfere with the debugger. This is achieved > by holding threadLock until the tracking is established by `trackAppResume()`. > > For symmetry the set of owned locks should be equal before/after calling > `blockOnDebuggerSuspend()` I think. Therefore handlerLock and threadLock are > acquired before. Ok, so you need to hold threadLock from the time `blockOnDebuggerSuspend()` is done waiting on it until after `trackAppResume()` is done, and since `trackAppResume()` needs to grab the handlerLock, and you need to grab the handerLock before the threadLock, you need to jump through some lock grabbing/release hoops. However, you are in fact releasing the threadLock for a short time in `blockOnDebuggerSuspend()` after the wait completes. Doesn't this expose the resumee to potential suspending after the wait has completed and before `trackAppResume()` has been called? - PR: https://git.openjdk.java.net/jdk/pull/5849
Re: RFR: 8274687: JDWP deadlocks if some Java thread reaches wait in blockOnDebuggerSuspend [v5]
> 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 https://github.com/openjdk/jdk/pull/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 Richard Reingruber has updated the pull request incrementally with two additional commits since the last revision: - Adding source filename and line numbers to printStack(). - Changes based on plummercj's comments. - Changes: - all: https://git.openjdk.java.net/jdk/pull/5849/files - new: https://git.openjdk.java.net/jdk/pull/5849/files/8c51e71f..806bceb8 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5849&range=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5849&range=03-04 Stats: 24 lines in 2 files changed: 12 ins; 5 del; 7 mod Patch: https://git.openjdk.java.net/jdk/pull/5849.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5849/head:pull/5849 PR: https://git.openjdk.java.net/jdk/pull/5849
Re: RFR: 8274687: JDWP deadlocks if some Java thread reaches wait in blockOnDebuggerSuspend [v4]
On Tue, 12 Oct 2021 22:55:05 GMT, Chris Plummer wrote: >> Richard Reingruber has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Improve @summary section of test. > > src/jdk.jdwp.agent/share/native/libjdwp/threadControl.c line 2200: > >> 2198: /* trackAppResume() needs handlerLock */ >> 2199: debugMonitorExit(threadLock); >> 2200: eventHandler_lock(); /* for proper lock order */ > > Is it still necessary for the handlerLock to be held when calling > `blockOnDebuggerSuspend()` (presuming you don't also add the new handlerLock > related code in it)? It seems that only the threadLock is needed so it can > then wait on it. > > The main thing you've done to fix this issue is defer the > `blockOnDebuggerSuspend()` to be done after `event_callback()` has released > the handlerLock, and to make it so `blockOnDebuggerSuspend()` does not block > while holding the handlerLock, so is there really any reason to be grabbing > it at all? Please see my answer on your [comment for L2220](https://github.com/openjdk/jdk/pull/5849#discussion_r727573811) > src/jdk.jdwp.agent/share/native/libjdwp/threadControl.c line 2220: > >> 2218: * suspends a thread it will remain suspended. >> 2219: */ >> 2220: trackAppResume(resumer); > > `trackAppResume()` doesn't really need the handlerLock. However, it will grab > it when calling `eventHandler_createInternalThreadOnly()`. Since you want to > make sure it is grabbed before threadLock in order to preserve lock ordering, > that complicates things if we decided not to grab the handlerLock in the code > above. What I'm now thinking is all that is really needed is to hold the > threadLock around the call to `blockOnDebuggerSuspend()`, or better yet just > grab it from within `blockOnDebuggerSuspend()`. You probably don't need to do > anything with handlerLock or threadLock inside of `doPendingTasks()`. After returning from `blockOnDebuggerSuspend()` we have to make sure resumee cannot be suspended by JDWP means otherwise the current thread which is about to execute j.l.Thread.resume() will interfere with the debugger. This is achieved by holding threadLock until the tracking is established by `trackAppResume()`. For symmetry the set of owned locks should be equal before/after calling `blockOnDebuggerSuspend()` I think. Therefore handlerLock and threadLock are acquired before. - PR: https://git.openjdk.java.net/jdk/pull/5849
Re: RFR: 8274687: JDWP deadlocks if some Java thread reaches wait in blockOnDebuggerSuspend [v4]
On Wed, 13 Oct 2021 07:06:17 GMT, Chris Plummer wrote: >> Richard Reingruber has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Improve @summary section of test. > > src/jdk.jdwp.agent/share/native/libjdwp/threadControl.c line 750: > >> 748: * handlerLock and threadLock are owned when returning and the >> suspendCount of >> 749: * the given thread is 0. >> 750: */ > > How about: > > /* > * The caller must own handlerLock and threadLock. > * If the suspendCount of the given thread is greater than 0, then the > * current thread will release the handlerLock and wait on the threadLock. It > * must release the handlerLock first, because threadControl_resumeThread() > * and threadControl_resumeAll() need it, and calling them is how suspendCount > * will eventually be decremented to 0. > * handlerLock and threadLock are owned when returning and the suspendCount of > * the given thread is 0. > */ Reads better. - PR: https://git.openjdk.java.net/jdk/pull/5849
Re: RFR: 8274687: JDWP deadlocks if some Java thread reaches wait in blockOnDebuggerSuspend [v4]
On Tue, 12 Oct 2021 21:45:03 GMT, Chris Plummer wrote:
>> Richard Reingruber has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Improve @summary section of test.
>
> test/jdk/com/sun/jdi/ResumeAfterThreadResumeCallTest.java line 100:
>
>> 98: // "resumee" is suspended now because of the breakpoint
>> 99: // Calling Thread.resume() will block this thread.
>> 100:
>
> no need for empty line here
Done.
> test/jdk/com/sun/jdi/ResumeAfterThreadResumeCallTest.java line 176:
>
>> 174: mainThreadReturnedFromResumeCall = ((PrimitiveValue)
>> v).booleanValue();
>> 175: if (!resumedResumee) {
>> 176: // main thread should be still blocked.
>
> "...should still be blocked"
Done.
-
PR: https://git.openjdk.java.net/jdk/pull/5849
Re: RFR: 8274687: JDWP deadlocks if some Java thread reaches wait in blockOnDebuggerSuspend [v4]
On Tue, 12 Oct 2021 21:39:35 GMT, Chris Plummer wrote:
>> Richard Reingruber has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Improve @summary section of test.
>
> test/jdk/com/sun/jdi/ResumeAfterThreadResumeCallTest.java line 159:
>
>> 157: ThreadReference resumee = bpe.thread();
>> 158: ObjectReference resumeeThreadObj =
>> resumee.frame(1).thisObject();
>> 159: printStack(resumee);
>
> As long as you're printing stacks, I think the stack of the main thread would
> be useful here, but you need to suspend it first. I don't think that
> interferes with the test.
> ```
> mainThread.suspend();
> printStack(mainThread);
> mainThread.resume();
I've added that.
> test/jdk/com/sun/jdi/ResumeAfterThreadResumeCallTest.java line 167:
>
>> 165: log("Sleeping 500ms shows that the main thread is blocked
>> calling Thread.resume() on \"resumee\" Thread.");
>> 166: Thread.sleep(500);
>> 167: log("After sleep.");
>
> And after line 167 is also a good place to print the main thread's stack.
Ok.
-
PR: https://git.openjdk.java.net/jdk/pull/5849
Re: RFR: 8274687: JDWP deadlocks if some Java thread reaches wait in blockOnDebuggerSuspend [v4]
On Tue, 12 Oct 2021 21:35:59 GMT, Chris Plummer wrote:
>> Richard Reingruber has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Improve @summary section of test.
>
> test/jdk/com/sun/jdi/ResumeAfterThreadResumeCallTest.java line 165:
>
>> 163: setField(resumeeThreadObj, "reachedBreakpoint",
>> vm().mirrorOf(true));
>> 164:
>> 165: log("Sleeping 500ms shows that the main thread is blocked
>> calling Thread.resume() on \"resumee\" Thread.");
>
> "shows" -> "so"
Ok.
-
PR: https://git.openjdk.java.net/jdk/pull/5849
Re: RFR: 8274687: JDWP deadlocks if some Java thread reaches wait in blockOnDebuggerSuspend [v4]
On Tue, 12 Oct 2021 21:06:00 GMT, Chris Plummer wrote:
>> Richard Reingruber has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Improve @summary section of test.
>
> test/jdk/com/sun/jdi/ResumeAfterThreadResumeCallTest.java line 81:
>
>> 79: System.out.println();
>> 80: System.out.println("###(Target,"+ threadName +") " + m);
>> 81: System.out.println();
>
> I'm not sure why you have the two extra `println()` calls. Seems to just add
> unneeded blank lines in the log file.
I'll remove them.
I think I copied the method from another test where I used a lot of jit
compiler tracing.
> test/jdk/com/sun/jdi/ResumeAfterThreadResumeCallTest.java line 222:
>
>> 220: System.out.println();
>> 221: System.out.println("###(Debugger) " + m);
>> 222: System.out.println();
>
> Same here with the extra `printlin()` calls
I'll remove them.
-
PR: https://git.openjdk.java.net/jdk/pull/5849
Re: RFR: 8274687: JDWP deadlocks if some Java thread reaches wait in blockOnDebuggerSuspend [v4]
On Tue, 12 Oct 2021 08:03:22 GMT, Richard Reingruber wrote: >> 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 https://github.com/openjdk/jdk/pull/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 > > Richard Reingruber has updated the pull request incrementally with one > additional commit since the last revision: > > Improve @summary section of test. src/jdk.jdwp.agent/share/native/libjdwp/threadControl.c line 750: > 748: * handlerLock and threadLock are owned when returning and the > suspendCount of > 749: * the given thread is 0. > 750: */ How about: /* * The caller must own handlerLock and threadLock. * If the suspendCount of the given thread is greater than 0, then the * current thread will release the handlerLock and wait on the threadLock. It * must release the handlerLock first, because threadControl_resumeThread() * and threadControl_resumeAll() need it, and calling them is how suspendCount * will eventually be decremented to 0. * handlerLock and threadLock are owned when returning and the suspendCount of * the given thread is 0. */ - PR: https://git.openjdk.java.net/jdk/pull/5849
Re: RFR: 8274687: JDWP deadlocks if some Java thread reaches wait in blockOnDebuggerSuspend [v4]
On Tue, 12 Oct 2021 08:03:22 GMT, Richard Reingruber wrote:
>> 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 https://github.com/openjdk/jdk/pull/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
>
> Richard Reingruber has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Improve @summary section of test.
src/jdk.jdwp.agent/share/native/libjdwp/threadControl.c line 2200:
> 2198: /* trackAppResume() needs handlerLock */
> 2199: debugMonitorExit(threadLock);
> 2200: eventHandler_lock(); /* for proper lock order */
Is it still necessary for the handlerLock to be held when calling
`blockOnDebuggerSuspend()` (presuming you don't also add the new handlerLock
related code in it)? It seems that only the threadLock is needed so it can then
wait on it.
The main thing you've done to fix this issue is defer the
`blockOnDebuggerSuspend()` to be done after `event_callback()` has released the
handlerLock, and to make it so `blockOnDebuggerSuspend()` does not block while
holding the handlerLock, so is there really any reason to be grabbing it at all?
src/jdk.jdwp.agent/share/native/libjdwp/threadControl.c line 2220:
> 2218: * suspends a thread it will remain suspended.
> 2219: */
> 2220: trackAppResume(resumer);
`trackAppResume()` doesn't really need the handlerLock. However, it will grab
it when calling `eventHandler_createInternalThreadOnly()`. Since you want to
make sure it is grabbed before threadLock in order to preserve lock ordering,
that complicates things if we decided not to grab the handlerLock in the code
above. What I'm now thinking is all that is really needed is to hold the
threadLock around the call to `blockOnDebuggerSuspend()`, or better yet just
grab it from within `blockOnDebuggerSuspend()`. You probably don't need to do
anything with handlerLock or threadLock inside of `doPendingTasks()`.
test/jdk/com/sun/jdi/ResumeAfterThreadResumeCallTest.java line 81:
> 79: System.out.println();
> 80: System.out.println("###(Target,"+ threadName +") " + m);
> 81: System.out.println();
I'm not sure why you have the two extra `println()` calls. Seems to just add
unneeded blank lines in the log file.
test/jdk/com/sun/jdi/ResumeAfterThreadResumeCallTest.java line 100:
> 98: // "resumee" is suspended now because of the breakpoint
> 99: // Calling Thread.resume() will block this thread.
> 100:
no need for empty line here
test/jdk/com/sun/jdi/ResumeAfterThreadResumeCallTest.java line 159:
> 157: ThreadReference resumee = bpe.thread();
> 158: ObjectReference resumeeThreadObj = resumee.frame(1).thisObject();
> 159: printStack(resumee);
As long as you're printing stacks, I think the stack of the main thread would
be useful here, but you need to suspend it first. I don't think that interferes
with the test.
```
mainThread.suspend();
printStack(mainThread);
mainThread.resume();
test/jdk/com/sun/jdi/ResumeAfterThreadResumeCallTest.java line 165:
> 163: setField(resumeeThreadObj, "reachedBreakpoint",
> vm().mirrorOf(true));
> 164:
> 165: log("Sleeping 500ms shows that the main thread is blocked
> calling Thread.resume() on \"resumee\" Thread.");
"shows" -> "so"
test/jdk/com/sun/jdi/ResumeAfterThreadResumeCallTest.java line 167:
> 165: log("Sleeping 500ms shows that the main thread is blocked
> calling Thread.resume() on \"resumee\" Thread.");
> 166: Thread.sleep(500);
> 167: log("After sleep.");
And after line 167 is also a good place to print the main thread's stack.
Re: RFR: 8274687: JDWP deadlocks if some Java thread reaches wait in blockOnDebuggerSuspend [v4]
On Tue, 12 Oct 2021 05:59:28 GMT, Chris Plummer wrote: >> Richard Reingruber has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Improve @summary section of test. > > test/jdk/com/sun/jdi/ResumeAfterThreadResumeCallTest.java line 31: > >> 29: * the JDWP agent (in blockOnDebuggerSuspend()) because it >> called >> 30: * j.l.Thread.resume() on a thread R that was suspended by the >> 31: * debugger. > > This is hard to follow. Maybe instead document the steps the test takes. For > example maybe something like: Suspend Thread R via breakpoint. Thread T calls > j.l.Thread.resume() on Thread R, resulting in Thread T blocking in > blockOnDebuggerSuspend. Resume Thread R using ThreadReference.resume(). > Verify that Thread T is no longer blocked in blockOnDebuggerSuspend. > > Also, it would be nice if thread names in the description matched the names > used in the implementation. Thanks for looking at this. I've updated the test summary. - PR: https://git.openjdk.java.net/jdk/pull/5849
Re: RFR: 8274687: JDWP deadlocks if some Java thread reaches wait in blockOnDebuggerSuspend [v4]
> 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 https://github.com/openjdk/jdk/pull/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 Richard Reingruber has updated the pull request incrementally with one additional commit since the last revision: Improve @summary section of test. - Changes: - all: https://git.openjdk.java.net/jdk/pull/5849/files - new: https://git.openjdk.java.net/jdk/pull/5849/files/e198e5ea..8c51e71f Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5849&range=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5849&range=02-03 Stats: 3 lines in 1 file changed: 2 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/5849.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5849/head:pull/5849 PR: https://git.openjdk.java.net/jdk/pull/5849
Re: RFR: 8274687: JDWP deadlocks if some Java thread reaches wait in blockOnDebuggerSuspend [v3]
> 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 https://github.com/openjdk/jdk/pull/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 Richard Reingruber has updated the pull request incrementally with one additional commit since the last revision: Improve @summary section of test. - Changes: - all: https://git.openjdk.java.net/jdk/pull/5849/files - new: https://git.openjdk.java.net/jdk/pull/5849/files/6cc006b1..e198e5ea Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5849&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5849&range=01-02 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/5849.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5849/head:pull/5849 PR: https://git.openjdk.java.net/jdk/pull/5849
Re: RFR: 8274687: JDWP deadlocks if some Java thread reaches wait in blockOnDebuggerSuspend [v2]
> 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 https://github.com/openjdk/jdk/pull/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 Richard Reingruber has updated the pull request incrementally with one additional commit since the last revision: Improve @summary section of test. - Changes: - all: https://git.openjdk.java.net/jdk/pull/5849/files - new: https://git.openjdk.java.net/jdk/pull/5849/files/8c1b5cec..6cc006b1 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5849&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5849&range=00-01 Stats: 30 lines in 1 file changed: 18 ins; 0 del; 12 mod Patch: https://git.openjdk.java.net/jdk/pull/5849.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5849/head:pull/5849 PR: https://git.openjdk.java.net/jdk/pull/5849
Re: RFR: 8274687: JDWP deadlocks if some Java thread reaches wait in blockOnDebuggerSuspend
On Thu, 7 Oct 2021 13:50:49 GMT, Richard Reingruber wrote: > 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 https://github.com/openjdk/jdk/pull/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 test/jdk/com/sun/jdi/ResumeAfterThreadResumeCallTest.java line 31: > 29: * the JDWP agent (in blockOnDebuggerSuspend()) because it called > 30: * j.l.Thread.resume() on a thread R that was suspended by the > 31: * debugger. This is hard to follow. Maybe instead document the steps the test takes. For example maybe something like: Suspend Thread R via breakpoint. Thread T calls j.l.Thread.resume() on Thread R, resulting in Thread T blocking in blockOnDebuggerSuspend. Resume Thread R using ThreadReference.resume(). Verify that Thread T is no longer blocked in blockOnDebuggerSuspend. Also, it would be nice if thread names in the description matched the names used in the implementation. - PR: https://git.openjdk.java.net/jdk/pull/5849
RFR: 8274687: JDWP deadlocks if some Java thread reaches wait in blockOnDebuggerSuspend
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 https://github.com/openjdk/jdk/pull/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 - Commit messages: - Remove delay in cleanup again. - Fix that prevents deadlocks. - New test that deadlocks target VM calling ThreadReference.resume(). - Delay cleanup after JDWP Dispose command to trigger deadlock in the test dispose003 Changes: https://git.openjdk.java.net/jdk/pull/5849/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=5849&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8274687 Stats: 280 lines in 2 files changed: 262 ins; 13 del; 5 mod Patch: https://git.openjdk.java.net/jdk/pull/5849.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5849/head:pull/5849 PR: https://git.openjdk.java.net/jdk/pull/5849
