RFR 8193879: Java debugger hangs on method invocation
Severin Gehwolf
sgehwolf at redhat.com
Tue Oct 9 15:50:30 UTC 2018
Hi Seguei,
On Mon, 2018-10-08 at 17:57 -0700, serguei.spitsyn at oracle.com wrote:
> Hi Severin,
>
> You already fixed a couple of deadlocks in the debugger method
> invocation and have an expertise in this area.
> Do you have any time to review the webrev from Daniil?
Sorry, not at the moment. It's also been a while since spelunking in
that area.
> Otherwise, I'd like to keep you aware about this fix.
OK, thanks! I'll try to have a look at it post-push.
Any particular reason why the bug is private? I don't seem to be able
to view it:
https://bugs.openjdk.java.net/browse/JDK-8193879
Thanks,
Severin
> Thanks,
> Serguei
>
>
> On 10/8/18 17:53, serguei.spitsyn at oracle.com wrote:
> > Hi Daniil,
> >
> > It seems to me, this fix is going to work.
> >
> > The freeze() method only cares there are no pending resume
> > commands:
> > 99 synchronized void freeze() {
> > 100 if (cache == null &&
> > (pendingResumeCommands.isEmpty())) {
> > 101 /*
> > 102 * No pending resumes to worry about. The VM is
> > suspended
> > 103 * and additional state can be cached. Notify all
> > 104 * interested listeners.
> > 105 */
> > 106 processVMAction(new VMAction(vm,
> > VMAction.VM_SUSPENDED));
> > 107 enableCache();
> > 108 }
> > 109 }
> > With new version of the notifyCommandComplete:
> > 95 void notifyCommandComplete(int id) {
> > 96 pendingResumeCommands.remove(id);
> > 97 }
> > a pending resume command can be deleted from the
> > pendingResumeCommands set.
> > This does not matter if the collection is already empty.
> >
> > The only other place for a potential conflict is the method:
> > 111 synchronized PacketStream thawCommand(CommandSender
> > sender) {
> > 112 PacketStream stream = sender.send();
> > 113 pendingResumeCommands.add(stream.id());
> > 114 thaw();
> > 115 return stream;
> > 116 }
> > However, there is no problem here as the pendingResumeCommands is a
> > synchronized set.
> >
> >
> > - return (BreakpointEvent)waitForRequestedEvent(request);
> > + return (BreakpointEvent) waitForRequestedEvent(request);
> > Not sure why have you added space after the cast.
> > We should not have it by coding convention.
> > Also, the local style does not have it as well.
> > Examples are:
> > 761 StepEvent retEvent =
> > (StepEvent)waitForRequestedEvent(sr);
> > 835 return (Location)locs.get(0);
> > 873 return
> > (ClassPrepareEvent)waitForRequestedEvent(request);
> >
> > Otherwise, it looks pretty good to me.
> > No need in another webrev if you fix the above.
> >
> > Thanks,
> > Serguei
> >
> >
> > On 10/4/18 14:19, Daniil Titov wrote:
> > > Hi Gary and Chris,
> > >
> > > Please review an updated version of the patch that has newly
> > > added test for the case when suspend policy is set to
> > > SUSPEND_EVENT_THREAD reimplemented using JDI API. Thus, the
> > > changes in
> > > src/jdk.jdi/share/classes/com/sun/tools/example/debug/tty/TTY.jav
> > > a are no longer required.
> > >
> > > I think vmInterrupted() is not invoked when suspend policy is set
> > > to SUSPEND_EVENT_THREAD to address the case when different
> > > threads keep hitting the same breakpoint and to avoid the
> > > switching the current thread in the background.
> > >
> > > The actual behavior of the debugger in the case when the
> > > breakpoint is hit and suspend policy is set to
> > > SUSPEND_EVENT_THREAD is just to print "Breakpoint hit:" in the
> > > output without adding any additional information or new line
> > > character. After that you need to set the current thread by
> > > entering "thread" command , e.g. "thread 1" and only then jdb
> > > prints the prompt (e.g. "main[1]") . The behavior looks as
> > > confusing since it is not obvious for the user that some input is
> > > expected from him (e.g. to set the current thread). I created a
> > > separated issue for that at
> > > https://bugs.openjdk.java.net/browse/JDK-8211736 .
> > >
> > > Webrev: http://cr.openjdk.java.net/~dtitov/8193879/webrev.02/
> > > Issue: https://bugs.openjdk.java.net/browse/JDK-8193879
> > >
> > > Thanks,
> > > --Daniil
> > >
> > > On 10/4/18, 10:28 AM, "Chris Plummer" <chris.plummer at oracle.com>
> > > wrote:
> > >
> > > On 10/4/18 5:12 AM, Gary Adams wrote:
> > > > In TTY.java do you need to force a simple prompt for the
> > > > breakpoint event output? What prevents currentThread from
> > > > being set at the time you are printing the prompt?
> > > >
> > > >
> > > > 103 // Print the prompt if suspend policy is
> > > > SUSPEND_EVENT_THREAD. In case of
> > > > 104 // SUSPEND_ALL policy this is handled by
> > > vmInterrupted()
> > > > method.
> > > > 105 if (be.request().suspendPolicy() ==
> > > > EventRequest.SUSPEND_EVENT_THREAD) {
> > > > 106 MessageOutput.println();
> > > > 107 MessageOutput.printPrompt();
> > > Normally the thread is suspended after the above code is
> > > executed:
> > >
> > > public void run() {
> > > EventQueue queue = Env.vm().eventQueue();
> > > while (connected) {
> > > try {
> > > EventSet eventSet = queue.remove();
> > > boolean resumeStoppedApp = false;
> > > EventIterator it = eventSet.eventIterator();
> > > while (it.hasNext()) {
> > > resumeStoppedApp |=
> > > !handleEvent(it.nextEvent());
> > > <--- calls the modified code above
> > > }
> > >
> > > if (resumeStoppedApp) {
> > > eventSet.resume();
> > > } else if (eventSet.suspendPolicy() ==
> > > EventRequest.SUSPEND_ALL) {
> > > setCurrentThread(eventSet); <------
> > > notifier.vmInterrupted();
> > > }
> > >
> > > However, it only calls setCurrentThread() for SUSPEND_ALL
> > > policies. So
> > > what Daniil has done here is make it print a simple prompt if
> > > the policy
> > > is SUSPEND_EVENT_THREAD. It's unclear to me what the actual
> > > debugger
> > > behavior is in this case. Don't we still suspend and get a
> > > prompt from
> > > which we can type in the next command? In this case, wouldn't
> > > you want a
> > > full prompt? Related to that question, why is vmInterrupted()
> > > only
> > > called if we suspend all threads, and not when we just
> > > suspend the
> > > thread that the breakpoint came in on?
> > >
> > > Chris
> > > >
> > > >
> > > > In Jdb.java you allow the waiting for the simple prompt.
> > > > I don't see waitForSimplePrompt used in any existing tests.
> > > > Since it is only looking for a single character it could
> > > > produce false positives if the '>' was produced in the
> > > > output stream. Is this wait paired to the added prompt for
> > > the
> > > > break point event?
> > > >
> > > > 236 return waitForSimplePrompt ?
> > > waitForSimplePrompt(1,
> > > > cmd.allowExit) : waitForPrompt(1, cmd.allowExit);
> > > >
> > > > It might be a good idea to include a test with multiple
> > > > threads each with a breakpoint that will trigger
> > > SUSPEND_EVENT_THREAD
> > > > behavior.
> > > >
> > > > On 10/4/18, 12:29 AM, Daniil Titov wrote:
> > > >> Please review the changes that fix the deadlock in the
> > > debugger when
> > > >> the debugger is running with the tracing option on.
> > > >>
> > > >> The problem here is that when the tracing is on the "JDI
> > > Target VM
> > > >> Interface" thread (the thread that reads all replies and
> > > then
> > > >> notifies the thread that sent the request that the reply
> > > has been
> > > >> received) is waiting for a lock which is already acquired
> > > by the
> > > >> thread that sent the request and is waiting for reply.
> > > >>
> > > >> The fix itself is in
> > > >> src/jdk.jdi/share/classes/com/sun/tools/jdi/VMState.java.
> > > >>
> > > >> The patch also reverts the changes done in 8129348
> > > "Debugger hangs in
> > > >> trace mode with TRACE_SENDS" in
> > > >>
> > > src/jdk.jdi/share/classes/com/sun/tools/jdi/InvokableTypeImpl.jav
> > > a
> > > >> since they address only a specific case (VM is suspended
> > > and static
> > > >> method is invoked) while the proposed fix also solves
> > > issue 8129348
> > > >> as well as issue 8193801 "Debugger hangs in trace mode for
> > > non-static
> > > >> methods".
> > > >>
> > > >> The changes include new tests for issues 8193879, 8193801
> > > and 8129348.
> > > >>
> > > >> The changes in
> > > >>
> > > src/jdk.jdi/share/classes/com/sun/tools/example/debug/tty/TTY.jav
> > > a
> > > >> solve the problem that the prompt is not printed in the
> > > debugger
> > > >> output when the breakpoint is hit and the suspend policy
> > > is
> > > >> SUSPEND_EVENT_THREAD. This is required for new tests to
> > > detect that
> > > >> command "stop thread at ..." is completed.
> > > >>
> > > >> Mach5 build and jdk_jdi tests succeeded.
> > > >>
> > > >> Webrev:
> > > http://cr.openjdk.java.net/~dtitov/8193879/webrev.01/
> > > >> Issue: https://bugs.openjdk.java.net/browse/JDK-8193879
> > > >>
> > > >> Thanks,
> > > >> --Daniil
> > > >>
> > > >>
> > > >>
> > > >
> > >
> > >
> > >
> > >
> >
>
More information about the serviceability-dev
mailing list