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