RFR 8193879: Java debugger hangs on method invocation

Daniil Titov daniil.x.titov at oracle.com
Tue Oct 9 02:12:00 UTC 2018

Hi Serguei,


Sure. That was my intention.




Bets regards,




From: "serguei.spitsyn at oracle.com" <serguei.spitsyn at oracle.com>
Date: Monday, October 8, 2018 at 6:17 PM
To: Daniil Titov <daniil.x.titov at oracle.com>, <gary.adams at oracle.com>, Alex Menkov <alexey.menkov at oracle.com>, Chris Plummer <chris.plummer at oracle.com>
Cc: serviceability-dev <serviceability-dev at openjdk.java.net>
Subject: Re: RFR 8193879: Java debugger hangs on method invocation



Could you wait a little bit for reply from Severin G.?


On 10/8/18 18:12, Daniil Titov wrote:

Hi Serguei, Chris, Alex, and Garry,


Thank you for reviewing this change. I will remove added space in test/jdk/com/sun/jdi/TestScaffold.java  resumeTo() method before pushing the change. It was accidentally added. 

-        return (BreakpointEvent)waitForRequestedEvent(request);
+        return (BreakpointEvent) waitForRequestedEvent(request);

Best regards,



From: "serguei.spitsyn at oracle.com" <serguei.spitsyn at oracle.com>
Date: Monday, October 8, 2018 at 5:53 PM
To: Daniil Titov <daniil.x.titov at oracle.com>, Chris Plummer <chris.plummer at oracle.com>, <gary.adams at oracle.com>
Cc: serviceability-dev <serviceability-dev at openjdk.java.net>
Subject: Re: RFR 8193879: Java debugger hangs on method invocation


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.


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.java 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
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) {
                     } else if (eventSet.suspendPolicy() == 
    EventRequest.SUSPEND_ALL) {
                         setCurrentThread(eventSet);   <------
    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?
    > 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.java 
    >> 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.java 
    >> 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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20181008/e38098e3/attachment-0001.html>

More information about the serviceability-dev mailing list