RFR 8193879: Java debugger hangs on method invocation
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Tue Oct 9 21:00:58 UTC 2018
Hi Severin,
On 10/9/18 8:50 AM, Severin Gehwolf wrote:
> 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.
Okay, thanks!
> 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
Most likely, it is because it is a shadow bug from the internal
bug tracking system reflecting external support.
These bugs are confidential by default.
this bug was from JetBrains folks.
If you want, I could send you the pieces you need separately.
Thanks,
Serguei
> 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