RFR 8211736: jdb doesn't print prompt when breakpoint is hit and suspend policy is STOP_EVENT_THREAD

[Alex Menkov]

Hi Daniil, Chris,

As far as I understand, the fix does not completely fixes all 
"non-atomic" output issues (at least TTY.executeCommand still prints 
prompt separately), so I agree that handle it as separate issue would be 
better.
Unfortunately I don't know Gary's ideas for the issue.

About the fix for print prompt:
1) TTY.java:
+            // Print current location if suspend policy is SUSPEND_NONE
I suppose you mean "Print breakpoint location"

2) Does it make sense to use printCurrentLocation for SUSPEND_EVENT_THREAD?
Is it expected to print something different from printBreakpointLocation?

3) I don't quite understand why we don't print simple prompt for 
SUSPEND_NONE. IIUC jdb can accept new command in both 
SUSPEND_EVENT_THREAD and SUSPEND_NONE cases (prompt shows that jdb waits 
for next command, right?)

4) JdbStopThreadTest.java
New line line in Java regexp is "\\R"

--alex

On 10/17/2018 10:47, Chris Plummer wrote:
> Hi Alex,
> 
> I think the tty buffering should be a separate bug fix, and I'd also 
> like input from Gary on it since he had tried something similar at one 
> point. It should probably also include a multithread test to prove the 
> fix is working (after first getting the test to fail without the changes).
> 
> thanks,
> 
> Chris
> 
> On 10/16/18 8:59 PM, Daniil Titov wrote:
>> Hi Alex, Chris and JC,
>>
>> Please review an updated version of the patch that addresses these 
>> issues.
>>
>> Webrev: http://cr.openjdk.java.net/~dtitov/8211736/webrev.03/
>> Issue:  https://bugs.openjdk.java.net/browse/JDK-8211736
>>
>> Thanks!
>> --Daniil
>>
>>
>> On 10/12/18, 9:52 AM, "Alex Menkov" <alexey.menkov at oracle.com> wrote:
>>
>>      Hi Daniil,
>>      1) +1 for printCurrentLocation when suspendPolicy != SUSPEND_ALL
>>      2) wrong indent in JdbStopThreadTest.java:
>>         36         import lib.jdb.JdbCommand;
>>         37         import lib.jdb.JdbTest;
>>      3) I think it would be better to make waitForSimplePrompt 
>> property of
>>      JdbCommand (like JdbCommand.allowExit)
>>      jdb.command(JdbCommand.run().replyIsSimplePrompt());
>>      looks much clearer than
>>      jdb.command(JdbCommand.run(), true);
>>      4) (TTY.java)
>>          MessageOutput.lnprint("Breakpoint hit:");
>>      +  // Print current location and prompt if suspend policy is
>>      SUSPEND_EVENT_THREAD.
>>      +  // In case of SUSPEND_ALL policy this is handled by 
>> vmInterrupted()
>>      method.
>>      +  if (be.request().suspendPolicy() == 
>> EventRequest.SUSPEND_EVENT_THREAD) {
>>      +      printCurrentLocation(ThreadInfo.getThreadInfo(be.thread()));
>>      +      MessageOutput.printPrompt();
>>      +  }
>>      We have 3 separate outputs.
>>      If we have several concurrent threads, we can easy get mess of 
>> outputs
>>      from different threads.
>>      I think it would be better to print everything in a single chunk.
>>      I suppose TTY has other places with similar "non-atomic" output, so
>>      maybe revising TTY output should be handled by separate issue.
>>      --alex
>>      On 10/11/2018 22:00, Chris Plummer wrote:
>>      > Hi Daniil,
>>      >
>>      > Can you send output from an example jdb session?
>>      >
>>      > Do you think maybe we should also call printCurrentLocation() 
>> when the
>>      > suspend policy is NONE?
>>      >
>>      > There's a typo in the following line. The space is on the wrong 
>> side of
>>      > the comma.
>>      >
>>      >    72         
>> jdb.command(JdbCommand.stopThreadAt(DEBUGGEE_CLASS ,bpLine));
>>      >
>>      > thanks,
>>      >
>>      > Chris
>>      >
>>      > On 10/11/18 8:02 PM, Daniil Titov wrote:
>>      >>
>>      >> Thank you,  JC!
>>      >>
>>      >> Please review an updated version of the patch that fixes newly 
>> added
>>      >> test for Windows platform  (now it uses system dependent line
>>      >> separator string).
>>      >>
>>      >> Webrev:http://cr.openjdk.java.net/~dtitov/8211736/webrev.02/
>>      >> <http://cr.openjdk.java.net/%7Edtitov/8211736/webrev.02/>
>>      >>
>>      >> Issue: https://bugs.openjdk.java.net/browse/JDK-8211736
>>      >>
>>      >> Best regards,
>>      >>
>>      >> --Daniil
>>      >>
>>      >> *From: *JC Beyler <jcbeyler at google.com>
>>      >> *Date: *Thursday, October 11, 2018 at 7:17 PM
>>      >> *To: *<daniil.x.titov at oracle.com>
>>      >> *Cc: *<serviceability-dev at openjdk.java.net>
>>      >> *Subject: *Re: RFR 8211736: jdb doesn't print prompt when 
>> breakpoint
>>      >> is hit and suspend policy is STOP_EVENT_THREAD
>>      >>
>>      >> Hi Daniil,
>>      >>
>>      >> Looks good to me. I saw a really small nit:
>>      >>
>>      >> 
>> http://cr.openjdk.java.net/~dtitov/8211736/webrev.01/test/jdk/com/sun/jdi/JdbStopThreadTest.java.html 
>>
>>      >> 
>> <http://cr.openjdk.java.net/%7Edtitov/8211736/webrev.01/test/jdk/com/sun/jdi/JdbStopThreadTest.java.html> 
>>
>>      >>
>>      >> 70  jdb.command(JdbCommand.stopThreadAt( DEBUGGEE_CLASS 
>> ,bpLine));
>>      >>
>>      >> There is a space after the '('.
>>      >>
>>      >> No need to send a new webrev for that evidently :),
>>      >>
>>      >> Jc
>>      >>
>>      >> On Thu, Oct 11, 2018 at 5:07 PM Daniil Titov
>>      >> <daniil.x.titov at oracle.com <mailto:daniil.x.titov at oracle.com>> 
>> wrote:
>>      >>
>>      >>     Please review the change that fixes the issue with missing 
>> prompt
>>      >>     in jdb when a breakpoint is hit and the suspend policy is 
>> set to
>>      >>     stop the thread only.
>>      >>
>>      >>     Webrev: http://cr.openjdk.java.net/~dtitov/8211736/webrev.01
>>      >>     <http://cr.openjdk.java.net/%7Edtitov/8211736/webrev.01>
>>      >>     Issue: https://bugs.openjdk.java.net/browse/JDK-8211736
>>      >>
>>      >>     Thanks!
>>      >>     --Daniil
>>      >>
>>      >>
>>      >>
>>      >> --
>>      >>
>>      >> Thanks,
>>      >>
>>      >> Jc
>>      >>
>>      >
>>
>>
> 
>