RFR: 8275259: Add support for Java level DCmd
I proposed to extend DCmd to allow Java developers to customize their own
diagnostic commands last week.
At present, I have implemented a preliminary version.
In the current implementation, I provided some simple APIs in the Java layer
(under sun.management.cmd) for defining and registering commands.
- Executable interface
User-defined commands need to implement this interface, the interface only
contains a simle method:
/**
* @param output the output when this executable is running
*/
void execute(PrintWriter output);
- Add two annotations (@Command and @Parameter) to describe the command meta
info
- Use Factory API to register command, the following forms are supported
@Command(name = "My.Echo", description = "Echo description")
class Echo implements Executable {
@Parameter(name = "text", ordinal=0, isMandatory = true)
String text;
@Parameter(name = "repeat", isMandatory = true, defaultValue = "1")
int repeat;
@Override
public void execute(PrintWriter out) {
for (int i = 0 ; i < repeat; i++) {
out.println(text);
}
}
}
Factory.register(Echo.class);
Factory.register("My.Date", output -> {
output.println(new Date());
});
- When the command is running, the VM will call `Executor.executeCommand` to
execute the command. In the implementation of Executor, I introduced a simple
timeout mechanism to prevent the command channel from being blocked.
At the VM layer, I extended the existing DCmd framework(implemented JavaDCmd
and JavaDCmdFactoryImpl) to be compatible with existing functions (jmx, help,
etc.).
In terms of security, considering that the SecurityManager will be deprecated,
there are not too many considerations for the time being.
Any input is appreciated.
Thanks,
Denghui
-
Commit messages:
- 8275259: Add support for Java level DCmd
Changes: https://git.openjdk.java.net/jdk/pull/5938/files
Webrev: https://webrevs.openjdk.java.net/?repo=jdk=5938=00
Issue: https://bugs.openjdk.java.net/browse/JDK-8275259
Stats: 1159 lines in 12 files changed: 1158 ins; 0 del; 1 mod
Patch: https://git.openjdk.java.net/jdk/pull/5938.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/5938/head:pull/5938
PR: https://git.openjdk.java.net/jdk/pull/5938
Re: RFR: 8275240: Change nested classes in jdk.attach to static nested classes
On Tue, 12 Oct 2021 07:20:14 GMT, Andrey Turbanov wrote: > Non-static classes hold a link to their parent classes, which in many cases > can be avoided. > Similar cleanup in java.base - > [JDK-8261880](https://bugs.openjdk.java.net/browse/JDK-8261880) A couple of files need a copyright year update. Otherwise, it looks good. Thanks, Serguei - Marked as reviewed by sspitsyn (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/5904
Re: RFR: 8275240: Change nested classes in jdk.attach to static nested classes
On Tue, 12 Oct 2021 07:20:14 GMT, Andrey Turbanov wrote: > Non-static classes hold a link to their parent classes, which in many cases > can be avoided. > Similar cleanup in java.base - > [JDK-8261880](https://bugs.openjdk.java.net/browse/JDK-8261880) Seems trivially fine. Thanks, David - Marked as reviewed by dholmes (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/5904
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: 8275240: Change nested classes in jdk.attach to static nested classes
On Tue, 12 Oct 2021 07:20:14 GMT, Andrey Turbanov wrote: > Non-static classes hold a link to their parent classes, which in many cases > can be avoided. > Similar cleanup in java.base - > [JDK-8261880](https://bugs.openjdk.java.net/browse/JDK-8261880) Marked as reviewed by cjplummer (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/5904
Integrated: JDK-8275244: Suppress warnings on non-serializable array component types in jdk.management
On Wed, 13 Oct 2021 19:58:43 GMT, Joe Darcy wrote: > After a refinement to the checks under development in #5709, the new checks > examine array types of serial fields and warn if the underlying component > type is not serializable. Per the JLS, all array types are serializable, but > if the base component is not serializable, the serialization process can > throw an exception. > > From "Java Object Serialization Specification: 2 - Object Output Classes": > > "If the object is an array, writeObject is called recursively to write the > ObjectStreamClass of the array. The handle for the array is assigned. It is > followed by the length of the array. Each element of the array is then > written to the stream, after which writeObject returns." > > The jdk.management module has an instance of this coding pattern that need > suppression to compile successfully under the future warning. This pull request has now been integrated. Changeset: d9e03e42 Author:Joe Darcy URL: https://git.openjdk.java.net/jdk/commit/d9e03e42afbb2e5115b67accfffad4938b8314b1 Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod 8275244: Suppress warnings on non-serializable array component types in jdk.management Reviewed-by: alanb - PR: https://git.openjdk.java.net/jdk/pull/5934
Re: RFR: JDK-8275244: Suppress warnings on non-serializable array component types in jdk.management
On Wed, 13 Oct 2021 19:58:43 GMT, Joe Darcy wrote: > After a refinement to the checks under development in #5709, the new checks > examine array types of serial fields and warn if the underlying component > type is not serializable. Per the JLS, all array types are serializable, but > if the base component is not serializable, the serialization process can > throw an exception. > > From "Java Object Serialization Specification: 2 - Object Output Classes": > > "If the object is an array, writeObject is called recursively to write the > ObjectStreamClass of the array. The handle for the array is assigned. It is > followed by the length of the array. Each element of the array is then > written to the stream, after which writeObject returns." > > The jdk.management module has an instance of this coding pattern that need > suppression to compile successfully under the future warning. Marked as reviewed by alanb (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/5934
RFR: JDK-8275244: Suppress warnings on non-serializable array component types in jdk.management
After a refinement to the checks under development in #5709, the new checks examine array types of serial fields and warn if the underlying component type is not serializable. Per the JLS, all array types are serializable, but if the base component is not serializable, the serialization process can throw an exception. >From "Java Object Serialization Specification: 2 - Object Output Classes": "If the object is an array, writeObject is called recursively to write the ObjectStreamClass of the array. The handle for the array is assigned. It is followed by the length of the array. Each element of the array is then written to the stream, after which writeObject returns." The jdk.management module has an instance of this coding pattern that need suppression to compile successfully under the future warning. - Commit messages: - JDK-8275244: Suppress warnings on non-serializable array component types in jdk.management Changes: https://git.openjdk.java.net/jdk/pull/5934/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=5934=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8275244 Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/5934.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5934/head:pull/5934 PR: https://git.openjdk.java.net/jdk/pull/5934
RFR: 8275240: Change nested classes in jdk.attach to static nested classes
Non-static classes hold a link to their parent classes, which in many cases can be avoided. Similar cleanup in java.base - [JDK-8261880](https://bugs.openjdk.java.net/browse/JDK-8261880) - Commit messages: - [PATCH] Change nested classes in jdk.attach to static nested classes where possible Changes: https://git.openjdk.java.net/jdk/pull/5904/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=5904=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8275240 Stats: 4 lines in 4 files changed: 0 ins; 0 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/5904.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5904/head:pull/5904 PR: https://git.openjdk.java.net/jdk/pull/5904
Re: RFR: 8266936: Add a finalization JFR event [v16]
> Greetings, > > Object.finalize() was deprecated in JDK9. There is an ongoing effort to > replace and mitigate Object.finalize() uses in the JDK libraries; please see > https://bugs.openjdk.java.net/browse/JDK-8253568 for more information. > > We also like to assist users in replacing and mitigating uses in non-JDK code. > > Hence, this changeset adds a periodic JFR event to help identify which > classes are overriding Object.finalize(). > > Thanks > Markus Markus Grönlund has refreshed the contents of this pull request, and previous commits have been removed. The incremental views will show differences compared to the previous content of the PR. The pull request contains two new commits since the last revision: - cleanup - rebased and adjusted for new lock rankings - Changes: - all: https://git.openjdk.java.net/jdk/pull/4731/files - new: https://git.openjdk.java.net/jdk/pull/4731/files/5359a793..96a9d6bf Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4731=15 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4731=14-15 Stats: 0 lines in 0 files changed: 0 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/4731.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4731/head:pull/4731 PR: https://git.openjdk.java.net/jdk/pull/4731
Re: RFR: 8266936: Add a finalization JFR event [v15]
> Greetings, > > Object.finalize() was deprecated in JDK9. There is an ongoing effort to > replace and mitigate Object.finalize() uses in the JDK libraries; please see > https://bugs.openjdk.java.net/browse/JDK-8253568 for more information. > > We also like to assist users in replacing and mitigating uses in non-JDK code. > > Hence, this changeset adds a periodic JFR event to help identify which > classes are overriding Object.finalize(). > > Thanks > Markus Markus Grönlund has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 44 commits: - Merge branch '8266936-alt' of github.com:mgronlun/jdk into 8266936-alt - symbols-unix - jvm.h and JVM_Entry - no precompiled headers and mtServiceability nmt classification - remove rehashing and rely on default grow_hint for table resize - mtStatistics - localize - cleanup - FinalizerStatistics - Model as finalizerService - ... and 34 more: https://git.openjdk.java.net/jdk/compare/124f8237...5359a793 - Changes: https://git.openjdk.java.net/jdk/pull/4731/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=4731=14 Stats: 1893 lines in 36 files changed: 1375 ins; 409 del; 109 mod Patch: https://git.openjdk.java.net/jdk/pull/4731.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4731/head:pull/4731 PR: https://git.openjdk.java.net/jdk/pull/4731
Re: RFR: 8275035: Clean up worker thread infrastructure
On Mon, 11 Oct 2021 09:21:21 GMT, Per Liden wrote: > I propose that we clean up our GangWorker/WorkGang and related classes, to > remove abstractions we no longer need (after CMS was removed, MutexDispatcher > was removed, Parallel is now using WorkGang, etc) and adjusting names as > follows: > > * Rename AbstractGangTask to WorkerTask > * Rename WorkGang to WorkerThreads > * Fold GangWorker into WorkerThread > * Fold WorkManager into WorkerThreads > * Move SubTaskDone and friends to a new workerUtils.hpp/cpp > > I've split things up into several commits to make it easier to review. > > Testing: Passes Tier 1-3 on all Oracle platforms. Marked as reviewed by ayang (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/5886
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=5849=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5849=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
Integrated: 8272992: Replace usages of Collections.sort with List.sort call in jdk.* modules
On Mon, 23 Aug 2021 21:08:05 GMT, Andrey Turbanov wrote: > Collections.sort is just a wrapper, so it is better to use an instance method > directly. > Affected modules: jdk.javadoc, jdk.jcmd, jdk.jconsole This pull request has now been integrated. Changeset: 5ffb5d10 Author:Andrey Turbanov Committer: Pavel Rappo URL: https://git.openjdk.java.net/jdk/commit/5ffb5d100f3383f9afaf20c8a659971522153505 Stats: 14 lines in 5 files changed: 0 ins; 3 del; 11 mod 8272992: Replace usages of Collections.sort with List.sort call in jdk.* modules Reviewed-by: cjplummer, prappo - PR: https://git.openjdk.java.net/jdk/pull/5230
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
Integrated: 8275075: Remove unnecessary conversion to String in jdk.hotspot.agent
On Sun, 3 Oct 2021 18:58:50 GMT, Andrey Turbanov wrote: > Cleanup unnecessary toString() calls when conversion will happen implicitly > anyway This pull request has now been integrated. Changeset: dcf428c7 Author:Andrey Turbanov Committer: Serguei Spitsyn URL: https://git.openjdk.java.net/jdk/commit/dcf428c7a74e568deaededfc11d3c4e1bf7821f2 Stats: 14 lines in 4 files changed: 0 ins; 1 del; 13 mod 8275075: Remove unnecessary conversion to String in jdk.hotspot.agent Reviewed-by: sspitsyn, cjplummer - PR: https://git.openjdk.java.net/jdk/pull/5800
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
