Re: RFR JDK-8031554: com/sun/tools/attach/BasicTests.java fails intermittently

[shanliang]

Jaroslav Bachorik wrote:

Hi Shanliang,

On 07/01/2014 09:47 PM, shanliang wrote:

I am still thinking to keep the original fix, because:
1) to throw InterruptedException does not fix the test failure, it might
give more info for diagnostics. If it was really caused by an
InterruptedException, then to fix the issue we still need to know who
could interrupt the test main thread, in which case and why, and whether
possible to ignore it (skip the test or try again?).


I'm sorry but I can't agree with this. The proposed patch does not add 
anything else than making the InterruptedException visible. Adding 
process.waitFor() will solve nothing as it is already called by 
ProcessTools.getOutput(process) while creating OutputAnalyzer.



2) the test library is used also by other tests and to modify it might
make new fail, it is better to concentrate at first on a single test
before knowing the exact cause.


I wouldn't expect new failures - when an InterruptedException was 
thrown the ProcessTools.executeTestJvm(args) returned null. Either the 
test would fail with NPE or custom assert - it makes no sense to work 
with "null" process.


IMO, this should be fixed in the test library.

Sorry I may miss something here, you suggested:
   "Either the result of ProcessTools.getOutput() should be checked for 
null to detect this situation or ProcessTools.getOutput() should throw a 
more aggressive exception when waitFor() gets interrupted."


We still need to do something when an InterruptedException happens, skip 
the test or retry? before making a decision we should know why there was 
an InterruptedException and in which case, I really do not have any 
idea, and I do not want to exclude other possibilities.


Yes what my fix does is to expose an InterruptedException if it happens, 
but it could make sure that it was really because of an 
InterruptedException.


About new failure, there could be a negative test  which expected a 
IllegalStateException --> failed OutputAnalyser, who knows.


Shanliang


-JB-



Shanliang

Christian Tornqvist wrote:


I can’t remember if there was a reason for doing it like this, but it
seems reasonable to not catch the InterruptedException in getOutput().

Thanks,

Christian

*From:*Staffan Larsen [mailto:staffan.lar...@oracle.com]
*Sent:* Friday, June 27, 2014 4:49 AM
*To:* shanliang
*Cc:* Jaroslav Bachorik; serviceability-dev@openjdk.java.net
serviceability-dev@openjdk.java.net; Christian Tornqvist
*Subject:* Re: RFR JDK-8031554: com/sun/tools/attach/BasicTests.java
fails intermittently

It does look suspicious to catch and ignore the InterruptedException,
especially since the OutputAnalyzer constructor will fail in this case.

Christian is the original author of these classes: do you know if
there is a good rationale for doing this? Or should we propagate the
InterruptedException?

Thanks,

/Staffan

On 26 jun 2014, at 17:24, shanliang mailto:shanliang.ji...@oracle.com>> wrote:



Jaroslav Bachorik wrote:

Hi Shanliang,

On 06/26/2014 03:15 PM, shanliang wrote:

Hi,

Today ProcessTools.executeProcess has the code:
new OutputAnalyzer(pb.start());

and OutputAnalyzer constructor calls immediately:
exitValue = process.exitValue();

the test got exception because the process did not end.


Are you sure about this?

The OutputAnalyzer constructor, before calling
process.exitValue(), calls ProcessTools.getOutput(process)
which actually does process.waitFor()

Good catch!


A probable explanation would be that process.waitFor() gets
interrupted without the target process actually ending.

Either the result of ProcessTools.getOutput() should be
checked for null to detect this situation or
ProcessTools.getOutput() should throw a more aggressive
exception when waitFor() gets interrupted.

It was possible beacause of an InterruptedException, but maybe a
Process issue too.

process.waitFor() was called by the test main thread, I am
wondering who wanted to interrupt this thread?

Not know why ProcessTools.getOutput() catches InterruptedException
and then returns null, are there some other tests dependent to
this behavior? otherwise better to not catch InterruptedException.

I think to keep this modification, it will allow us to get more
information if the bug is reproduced, if getting an
InterruptedException then we need to do more investigation for
why, if no exception then we may rise a question to 
process.waitFor().


Shanliang


-JB-



So a direct solution for the test is not to call:
   ProcessTools.executeTestJvm(args);

but:
ProcessBuilder pb =

ProcessTools.createJavaProcessBuilder(Utils.addTestJavaOpts(args));

Process process = pb.start();

Re: RFR JDK-8031554: com/sun/tools/attach/BasicTests.java fails intermittently

[Jaroslav Bachorik]

Hi Shanliang,

On 07/01/2014 09:47 PM, shanliang wrote:

I am still thinking to keep the original fix, because:
1) to throw InterruptedException does not fix the test failure, it might
give more info for diagnostics. If it was really caused by an
InterruptedException, then to fix the issue we still need to know who
could interrupt the test main thread, in which case and why, and whether
possible to ignore it (skip the test or try again?).


I'm sorry but I can't agree with this. The proposed patch does not add 
anything else than making the InterruptedException visible. Adding 
process.waitFor() will solve nothing as it is already called by 
ProcessTools.getOutput(process) while creating OutputAnalyzer.



2) the test library is used also by other tests and to modify it might
make new fail, it is better to concentrate at first on a single test
before knowing the exact cause.


I wouldn't expect new failures - when an InterruptedException was thrown 
the ProcessTools.executeTestJvm(args) returned null. Either the test 
would fail with NPE or custom assert - it makes no sense to work with 
"null" process.


IMO, this should be fixed in the test library.

-JB-



Shanliang

Christian Tornqvist wrote:


I can’t remember if there was a reason for doing it like this, but it
seems reasonable to not catch the InterruptedException in getOutput().

Thanks,

Christian

*From:*Staffan Larsen [mailto:staffan.lar...@oracle.com]
*Sent:* Friday, June 27, 2014 4:49 AM
*To:* shanliang
*Cc:* Jaroslav Bachorik; serviceability-dev@openjdk.java.net
serviceability-dev@openjdk.java.net; Christian Tornqvist
*Subject:* Re: RFR JDK-8031554: com/sun/tools/attach/BasicTests.java
fails intermittently

It does look suspicious to catch and ignore the InterruptedException,
especially since the OutputAnalyzer constructor will fail in this case.

Christian is the original author of these classes: do you know if
there is a good rationale for doing this? Or should we propagate the
InterruptedException?

Thanks,

/Staffan

On 26 jun 2014, at 17:24, shanliang mailto:shanliang.ji...@oracle.com>> wrote:



Jaroslav Bachorik wrote:

Hi Shanliang,

On 06/26/2014 03:15 PM, shanliang wrote:

Hi,

Today ProcessTools.executeProcess has the code:
new OutputAnalyzer(pb.start());

and OutputAnalyzer constructor calls immediately:
exitValue = process.exitValue();

the test got exception because the process did not end.


Are you sure about this?

The OutputAnalyzer constructor, before calling
process.exitValue(), calls ProcessTools.getOutput(process)
which actually does process.waitFor()

Good catch!


A probable explanation would be that process.waitFor() gets
interrupted without the target process actually ending.

Either the result of ProcessTools.getOutput() should be
checked for null to detect this situation or
ProcessTools.getOutput() should throw a more aggressive
exception when waitFor() gets interrupted.

It was possible beacause of an InterruptedException, but maybe a
Process issue too.

process.waitFor() was called by the test main thread, I am
wondering who wanted to interrupt this thread?

Not know why ProcessTools.getOutput() catches InterruptedException
and then returns null, are there some other tests dependent to
this behavior? otherwise better to not catch InterruptedException.

I think to keep this modification, it will allow us to get more
information if the bug is reproduced, if getting an
InterruptedException then we need to do more investigation for
why, if no exception then we may rise a question to process.waitFor().

Shanliang


-JB-



So a direct solution for the test is not to call:
   ProcessTools.executeTestJvm(args);

but:
ProcessBuilder pb =
ProcessTools.createJavaProcessBuilder(Utils.addTestJavaOpts(args));
Process process = pb.start();
process.waitFor();
OutputAnalyzer output = new OutputAnalyzer(process);

here we do waiting:
process.waitFor();
before constructing an OutputAnalyzer.

ProcessTools is a tool class we may have same issue for
other tests
using this class. So we may need to improve the test library.

bug: https://bugs.openjdk.java.net/browse/JDK-8031554
webrev: http://cr.openjdk.java.net/~sjiang/JDK-8031554/00/



Thanks,
Shanliang

Re: RFR JDK-8031554: com/sun/tools/attach/BasicTests.java fails intermittently

[shanliang]

I am still thinking to keep the original fix, because:
1) to throw InterruptedException does not fix the test failure, it might 
give more info for diagnostics. If it was really caused by an 
InterruptedException, then to fix the issue we still need to know who 
could interrupt the test main thread, in which case and why, and whether 
possible to ignore it (skip the test or try again?).
2) the test library is used also by other tests and to modify it might 
make new fail, it is better to concentrate at first on a single test 
before knowing the exact cause.


Shanliang

Christian Tornqvist wrote:


I can't remember if there was a reason for doing it like this, but it 
seems reasonable to not catch the InterruptedException in getOutput().


 


Thanks,

Christian

 


*From:* Staffan Larsen [mailto:staffan.lar...@oracle.com]
*Sent:* Friday, June 27, 2014 4:49 AM
*To:* shanliang
*Cc:* Jaroslav Bachorik; serviceability-dev@openjdk.java.net 
serviceability-dev@openjdk.java.net; Christian Tornqvist
*Subject:* Re: RFR JDK-8031554: com/sun/tools/attach/BasicTests.java 
fails intermittently


 

It does look suspicious to catch and ignore the InterruptedException, 
especially since the OutputAnalyzer constructor will fail in this case. 

 

Christian is the original author of these classes: do you know if 
there is a good rationale for doing this? Or should we propagate the 
InterruptedException?


 


Thanks,

/Staffan

 

On 26 jun 2014, at 17:24, shanliang > wrote:




Jaroslav Bachorik wrote:

Hi Shanliang,

On 06/26/2014 03:15 PM, shanliang wrote:

Hi,

Today ProcessTools.executeProcess has the code:
new OutputAnalyzer(pb.start());

and OutputAnalyzer constructor calls immediately:
exitValue = process.exitValue();

the test got exception because the process did not end.


Are you sure about this?

The OutputAnalyzer constructor, before calling
process.exitValue(), calls ProcessTools.getOutput(process)
which actually does process.waitFor()

Good catch!


A probable explanation would be that process.waitFor() gets
interrupted without the target process actually ending.

Either the result of ProcessTools.getOutput() should be
checked for null to detect this situation or
ProcessTools.getOutput() should throw a more aggressive
exception when waitFor() gets interrupted.

It was possible beacause of an InterruptedException, but maybe a
Process issue too.

process.waitFor() was called by the test main thread, I am
wondering who wanted to interrupt this thread?

Not know why ProcessTools.getOutput() catches InterruptedException
and then returns null, are there some other tests dependent to
this behavior? otherwise better to not catch InterruptedException.

I think to keep this modification, it will allow us to get more
information if the bug is reproduced, if getting an
InterruptedException then we need to do more investigation for
why, if no exception then we may rise a question to process.waitFor().

Shanliang


-JB-



So a direct solution for the test is not to call:
   ProcessTools.executeTestJvm(args);

but:
ProcessBuilder pb =
ProcessTools.createJavaProcessBuilder(Utils.addTestJavaOpts(args));
Process process = pb.start();
process.waitFor();
OutputAnalyzer output = new OutputAnalyzer(process);

here we do waiting:
process.waitFor();
before constructing an OutputAnalyzer.

ProcessTools is a tool class we may have same issue for
other tests
using this class. So we may need to improve the test library.

bug: https://bugs.openjdk.java.net/browse/JDK-8031554
webrev: http://cr.openjdk.java.net/~sjiang/JDK-8031554/00/



Thanks,
Shanliang

 

Re: RFR(S): 8048892: TEST_BUG: shell script tests need to be change to not use retired @debuggeeVMOptions mechanism

[Jaroslav Bachorik]

Looks fine.

-JB-

On 07/01/2014 05:23 PM, Yekaterina Kantserova wrote:

Hi,

Could I please have a review of this fix.

webrev: http://cr.openjdk.java.net/~ykantser/8048892/webrev.00/
bug: https://bugs.openjdk.java.net/browse/JDK-8048892

The fix is verified internally on all basic platforms. I have also taken the 
opportunity to re-write com/sun/jdi/SuspendNoFlagTest.sh in java using 
testlibrary's functionality in order to make the test more compact and easier 
to maintain.

Thanks,
Katja

Re: RFR: 8046883 com/sun/jdi/ProcessAttachTest.sh gets "java.io.IOException: Invalid process identifier" on windows

[Yekaterina Kantserova]

Oh, great, than no worries. 

// Katja 

- Original Message - 
From: staffan.lar...@oracle.com 
To: yekaterina.kantser...@oracle.com 
Cc: serviceability-dev@openjdk.java.net 
Sent: Tuesday, July 1, 2014 5:20:57 PM GMT +01:00 Amsterdam / Berlin / Bern / 
Rome / Stockholm / Vienna 
Subject: Re: RFR: 8046883 com/sun/jdi/ProcessAttachTest.sh gets 
"java.io.IOException: Invalid process identifier" on windows 


Katja, 


I intentionally did not include javaoptions when launching the new JVMs since I 
could not think of any options that would affect how -agentlib:jdwp=suspend=y|n 
works or does not work (which is what this test verifies). 


/Staffan 





On 1 jul 2014, at 17:03, Yekaterina Kantserova < 
yekaterina.kantser...@oracle.com > wrote: 




Staffan, 

since I'm working on JDK-8048892 right now I happen to know 
test/com/sun/jdi/ProcessAttachTest.sh has used @debuggeeVMOptions. 

ProcessBuilder pb = ProcessTools.createJavaProcessBuilder(...) 

will not add test.vm.options and test.java.options to the command line. You 
need to use: 

ProcessBuilder pb = 
ProcessTools.createJavaProcessBuilder(Utils.addTestJavaOpts(...)) 

You can look at the ProcessTools.executeTestJvm() for more information. 

Thanks, 
Katja 



- Original Message - 
From: daniel.daughe...@oracle.com 
To: staffan.lar...@oracle.com 
Cc: serviceability-dev@openjdk.java.net 
Sent: Tuesday, July 1, 2014 3:37:38 PM GMT +01:00 Amsterdam / Berlin / Bern / 
Rome / Stockholm / Vienna 
Subject: Re: RFR: 8046883 com/sun/jdi/ProcessAttachTest.sh gets 
"java.io.IOException: Invalid process identifier" on windows 


On 6/23/14 2:33 AM, Staffan Larsen wrote: 


Fancy! 


new review: http://cr.openjdk.java.net/~sla/8046883/webrev.01/ 
test/com/sun/jdi/ProcessAttachTest.java 
New test looks very good. There's a couple of really long lines, 
but I can't think of a way to break the lines that makes sense 
with this new streams coding style. 

Thumbs up. 

Dan 







/Staffan 



On 18 jun 2014, at 13:59, Peter Allwin < peter.all...@oracle.com > wrote: 



This looks a lot better! 


(Since we’re using fancy new features we could use streams to find the 
connector instance) 



AttachingConnector ac = Bootstrap.virtualMachineManager().attachingConnectors() 
.stream() 
.filter(c -> c.name().equals( "com.sun.jdi.ProcessAttach" )) 
.findFirst() 
.orElseThrow(() -> new RuntimeException( "Unable to locate 
ProcessAttachingConnector" )); 


Thanks! 
/peter 



On 17 Jun 2014, at 19:46, Staffan Larsen < staffan.lar...@oracle.com > wrote: 



Here is a rewrite of the test in Java instead of a shell script. Should be 
easier to maintain. 


webrev: http://cr.openjdk.java.net/~sla/8046883/webrev.00/ 



Thanks, 
/Staffan 



On 17 jun 2014, at 15:12, Staffan Larsen < staffan.lar...@oracle.com > wrote: 




On 17 jun 2014, at 15:03, Alan Bateman < alan.bate...@oracle.com > wrote: 



On 17/06/2014 13:35, Staffan Larsen wrote: 


: 

It could be a timing issue, but in the other direction. If cygwin hasn’t yet 
started the real windows process when I run ps, then maybe ps will not list it. 
But given the “sleep 2” before the ps invocation, the process should have had 
time to started. No guarantees of course. 

Making the sleep shorter will not help as the process we are starting will not 
terminate until we tell it to. 


Okay, although what I was suggesting is to use your patch but additionally move 
the sleep at L79 into the new while loop so that it doesn't spin quickly 
through the 10 iterations. That would give the test 10 attempts (and 10 
seconds) to get the pid. 

Ah, I see. I misunderstood your comment. 

I started looking at rewriting the test in pure Java instead of the shell 
script. With the new Process.getPid() this looks like the best approach. I’ll 
come back with a new review request soon. 

/Staffan 

Re: RFR(S): 8048892: TEST_BUG: shell script tests need to be change to not use retired @debuggeeVMOptions mechanism

[Staffan Larsen]

Looks good!

Thanks,
/Staffan

On 1 jul 2014, at 17:23, Yekaterina Kantserova 
 wrote:

> Hi,
> 
> Could I please have a review of this fix.
> 
> webrev: http://cr.openjdk.java.net/~ykantser/8048892/webrev.00/
> bug: https://bugs.openjdk.java.net/browse/JDK-8048892
> 
> The fix is verified internally on all basic platforms. I have also taken the 
> opportunity to re-write com/sun/jdi/SuspendNoFlagTest.sh in java using 
> testlibrary's functionality in order to make the test more compact and easier 
> to maintain.
> 
> Thanks,
> Katja

RFR(S): 8048892: TEST_BUG: shell script tests need to be change to not use retired @debuggeeVMOptions mechanism

[Yekaterina Kantserova]

Hi,

Could I please have a review of this fix.

webrev: http://cr.openjdk.java.net/~ykantser/8048892/webrev.00/
bug: https://bugs.openjdk.java.net/browse/JDK-8048892

The fix is verified internally on all basic platforms. I have also taken the 
opportunity to re-write com/sun/jdi/SuspendNoFlagTest.sh in java using 
testlibrary's functionality in order to make the test more compact and easier 
to maintain.

Thanks,
Katja

Re: RFR: 8046883 com/sun/jdi/ProcessAttachTest.sh gets "java.io.IOException: Invalid process identifier" on windows

[Staffan Larsen]

Katja,

I intentionally did not include javaoptions when launching the new JVMs since I 
could not think of any options that would affect how -agentlib:jdwp=suspend=y|n 
works or does not work (which is what this test verifies).

/Staffan


On 1 jul 2014, at 17:03, Yekaterina Kantserova 
 wrote:

> Staffan,
> 
> since I'm working on JDK-8048892 right now I happen to know 
> test/com/sun/jdi/ProcessAttachTest.sh has used @debuggeeVMOptions.
> 
> ProcessBuilder pb = ProcessTools.createJavaProcessBuilder(...) 
> 
> will not add test.vm.options and test.java.options to the command line. You 
> need to use:
> 
> ProcessBuilder pb = 
> ProcessTools.createJavaProcessBuilder(Utils.addTestJavaOpts(...)) 
> 
> You can look at the ProcessTools.executeTestJvm() for more information.
> 
> Thanks,
> Katja
> 
> 
> 
> - Original Message -
> From: daniel.daughe...@oracle.com
> To: staffan.lar...@oracle.com
> Cc: serviceability-dev@openjdk.java.net
> Sent: Tuesday, July 1, 2014 3:37:38 PM GMT +01:00 Amsterdam / Berlin / Bern / 
> Rome / Stockholm / Vienna
> Subject: Re: RFR: 8046883 com/sun/jdi/ProcessAttachTest.sh gets 
> "java.io.IOException: Invalid process identifier" on windows
> 
> On 6/23/14 2:33 AM, Staffan Larsen wrote:
> Fancy!
> 
> new review: http://cr.openjdk.java.net/~sla/8046883/webrev.01/
>  
> test/com/sun/jdi/ProcessAttachTest.java
> New test looks very good. There's a couple of really long lines,
> but I can't think of a way to break the lines that makes sense
> with this new streams coding style.
> 
> Thumbs up.
>  
> Dan
> 
> 
> 
> /Staffan
> 
> On 18 jun 2014, at 13:59, Peter Allwin  wrote:
> 
> This looks a lot better!
> 
> (Since we’re using fancy new features we could use streams to find the 
> connector instance)
> 
> AttachingConnector ac = 
> Bootstrap.virtualMachineManager().attachingConnectors()
> .stream()
> .filter(c -> c.name().equals("com.sun.jdi.ProcessAttach"))
> .findFirst()
> .orElseThrow(() -> new RuntimeException("Unable to locate 
> ProcessAttachingConnector"));
> 
> Thanks!
> /peter
> 
> On 17 Jun 2014, at 19:46, Staffan Larsen  wrote:
> 
> Here is a rewrite of the test in Java instead of a shell script. Should be 
> easier to maintain.
> 
> webrev: http://cr.openjdk.java.net/~sla/8046883/webrev.00/
> 
> Thanks,
> /Staffan
> 
> On 17 jun 2014, at 15:12, Staffan Larsen  wrote:
> 
> 
> On 17 jun 2014, at 15:03, Alan Bateman  wrote:
> 
> On 17/06/2014 13:35, Staffan Larsen wrote:
> :
> 
> It could be a timing issue, but in the other direction. If cygwin hasn’t yet 
> started the real windows process when I run ps, then maybe ps will not list 
> it. But given the “sleep 2” before the ps invocation, the process should have 
> had time to started. No guarantees of course.
> 
> Making the sleep shorter will not help as the process we are starting will 
> not terminate until we tell it to.
> 
> 
> Okay, although what I was suggesting is to use your patch but additionally 
> move the sleep at L79 into the new while loop so that it doesn't spin quickly 
> through the 10 iterations. That would give the test 10 attempts (and 10 
> seconds) to get the pid.
> 
> Ah, I see. I misunderstood your comment.
> 
> I started looking at rewriting the test in pure Java instead of the shell 
> script. With the new Process.getPid() this looks like the best approach. I’ll 
> come back with a new review request soon.
> 
> /Staffan

Re: RFR: 8046883 com/sun/jdi/ProcessAttachTest.sh gets "java.io.IOException: Invalid process identifier" on windows

[Yekaterina Kantserova]

Staffan, 

since I'm working on JDK-8048892 right now I happen to know 
test/com/sun/jdi/ProcessAttachTest.sh has used @debuggeeVMOptions. 

ProcessBuilder pb = ProcessTools.createJavaProcessBuilder(...) 

will not add test.vm.options and test.java.options to the command line. You 
need to use: 

ProcessBuilder pb = 
ProcessTools.createJavaProcessBuilder(Utils.addTestJavaOpts(...)) 

You can look at the ProcessTools.executeTestJvm() for more information. 

Thanks, 
Katja 



- Original Message - 
From: daniel.daughe...@oracle.com 
To: staffan.lar...@oracle.com 
Cc: serviceability-dev@openjdk.java.net 
Sent: Tuesday, July 1, 2014 3:37:38 PM GMT +01:00 Amsterdam / Berlin / Bern / 
Rome / Stockholm / Vienna 
Subject: Re: RFR: 8046883 com/sun/jdi/ProcessAttachTest.sh gets 
"java.io.IOException: Invalid process identifier" on windows 


On 6/23/14 2:33 AM, Staffan Larsen wrote: 


Fancy! 


new review: http://cr.openjdk.java.net/~sla/8046883/webrev.01/ 
test/com/sun/jdi/ProcessAttachTest.java 
New test looks very good. There's a couple of really long lines, 
but I can't think of a way to break the lines that makes sense 
with this new streams coding style. 

Thumbs up. 

Dan 







/Staffan 



On 18 jun 2014, at 13:59, Peter Allwin < peter.all...@oracle.com > wrote: 



This looks a lot better! 


(Since we’re using fancy new features we could use streams to find the 
connector instance) 



AttachingConnector ac = Bootstrap.virtualMachineManager().attachingConnectors() 
.stream() 
.filter(c -> c.name().equals( "com.sun.jdi.ProcessAttach" )) 
.findFirst() 
.orElseThrow(() -> new RuntimeException( "Unable to locate 
ProcessAttachingConnector" )); 


Thanks! 
/peter 



On 17 Jun 2014, at 19:46, Staffan Larsen < staffan.lar...@oracle.com > wrote: 



Here is a rewrite of the test in Java instead of a shell script. Should be 
easier to maintain. 


webrev: http://cr.openjdk.java.net/~sla/8046883/webrev.00/ 



Thanks, 
/Staffan 



On 17 jun 2014, at 15:12, Staffan Larsen < staffan.lar...@oracle.com > wrote: 




On 17 jun 2014, at 15:03, Alan Bateman < alan.bate...@oracle.com > wrote: 



On 17/06/2014 13:35, Staffan Larsen wrote: 


: 

It could be a timing issue, but in the other direction. If cygwin hasn’t yet 
started the real windows process when I run ps, then maybe ps will not list it. 
But given the “sleep 2” before the ps invocation, the process should have had 
time to started. No guarantees of course. 

Making the sleep shorter will not help as the process we are starting will not 
terminate until we tell it to. 


Okay, although what I was suggesting is to use your patch but additionally move 
the sleep at L79 into the new while loop so that it doesn't spin quickly 
through the 10 iterations. That would give the test 10 attempts (and 10 
seconds) to get the pid. 

Ah, I see. I misunderstood your comment. 

I started looking at rewriting the test in pure Java instead of the shell 
script. With the new Process.getPid() this looks like the best approach. I’ll 
come back with a new review request soon. 

/Staffan 

Re: RFR: 8046883 com/sun/jdi/ProcessAttachTest.sh gets "java.io.IOException: Invalid process identifier" on windows

[Daniel D. Daugherty]

On 6/23/14 2:33 AM, Staffan Larsen wrote:

Fancy!

new review: http://cr.openjdk.java.net/~sla/8046883/webrev.01/ 



test/com/sun/jdi/ProcessAttachTest.java
New test looks very good. There's a couple of really long lines,
but I can't think of a way to break the lines that makes sense
with this new streams coding style.

Thumbs up.

Dan




/Staffan

On 18 jun 2014, at 13:59, Peter Allwin > wrote:



This looks a lot better!

(Since we’re using fancy new features we could use streams to find 
the connector instance)


AttachingConnector ac = 
Bootstrap.virtualMachineManager().attachingConnectors()

.stream()
.filter(c -> c.name().equals("com.sun.jdi.ProcessAttach"))
.findFirst()
.orElseThrow(() -> new RuntimeException("Unable to locate 
ProcessAttachingConnector"));


Thanks!
/peter

On 17 Jun 2014, at 19:46, Staffan Larsen > wrote:


Here is a rewrite of the test in Java instead of a shell script. 
Should be easier to maintain.


webrev: http://cr.openjdk.java.net/~sla/8046883/webrev.00/ 



Thanks,
/Staffan

On 17 jun 2014, at 15:12, Staffan Larsen > wrote:




On 17 jun 2014, at 15:03, Alan Bateman > wrote:



On 17/06/2014 13:35, Staffan Larsen wrote:

:

It could be a timing issue, but in the other direction. If cygwin 
hasn’t yet started the real windows process when I run ps, then 
maybe ps will not list it. But given the “sleep 2” before the ps 
invocation, the process should have had time to started. No 
guarantees of course.


Making the sleep shorter will not help as the process we are 
starting will not terminate until we tell it to.



Okay, although what I was suggesting is to use your patch but 
additionally move the sleep at L79 into the new while loop so that 
it doesn't spin quickly through the 10 iterations. That would give 
the test 10 attempts (and 10 seconds) to get the pid.


Ah, I see. I misunderstood your comment.

I started looking at rewriting the test in pure Java instead of the 
shell script. With the new Process.getPid() this looks like the 
best approach. I’ll come back with a new review request soon.


/Staffan

Re: RFR (XS) fix for a safepoint deadlock (8047720)

[Daniel D. Daugherty]

On 6/30/14 8:29 PM, David Holmes wrote:

Hi Dan,

I disagree on a few points but lets wait to see what Markus' work shows.


Thanks.


Though I would be surprised if it detects the indirect safepoint 
blocking caused by submitting a synchronous, safepoint-needing, VM op.


I did not mean to imply that Markus' work would have helped find
this bug. However, in fixing this bug I didn't want to add something
that Markus would later want to re-implement in a different way.

Dan




David

On 1/07/2014 3:08 AM, Daniel D. Daugherty wrote:

David,

Thanks for the review. Obviously I can't list you as a reviewer
since I already pushed the changeset...


On 6/29/14 11:54 PM, David Holmes wrote:

Correction ...

On 30/06/2014 3:33 PM, David Holmes wrote:

Hi Dan,

I see this has already gone in but I think it is worth looking 
closer at

this.

On 28/06/2014 2:18 AM, Daniel D. Daugherty wrote:

Greetings,

I have a fix ready for the following bug:

 8047720 Xprof hangs on Solaris
 https://bugs.openjdk.java.net/browse/JDK-8047720

Here is the webrev URL:

http://cr.openjdk.java.net/~dcubed/8047720-webrev/0-jdk9-hs-rt/

This deadlock occurred between the following threads:

 Main thread   - Trying to stop the WatcherThread as part of
 shutting down the VM; this thread is blocked
 on the PeriodicTask_lock which keeps it from
 reaching a safepoint.
 WatcherThread - Requested a VM_ForceSafepoint to complete
 a JavaThread::java_suspend() call as part
 of a FlatProfiler record_thread_ticks()
 call; this thread owns the PeriodicTask_lock
 since it is processing a periodic task.
 VMThread  - Trying to start a safepoint; this thread is
 blocked waiting for the Main thread to reach
 a safepoint.

The PeriodicTask_lock is one of the VM internal locks and is
typically managed using Mutex::_no_safepoint_check_flag to
avoid deadlocks. Yes, the irony is dripping on the floor... :-)


What was overlooked here is that the holder of a lock that is acquired
without safepoint checks, must never block at a safepoint whilst 
holding

that lock.


I'm not sure about the above rule when you're talking about
internal VM threads that are really JavaThreads. JavaThreads
can run into all kinds of our special logic when doing thread
state transitions and the like. JavaThreads can also post JVM/TI
events which can lead to blocking at safepoints or blocking on
JVM/TI raw monitors in event handlers, etc...




In this case the blocking is indirect, caused by the
synchronous nature of the VM_Operation, rather than a direct result of
"blocking for the safepoint" (which the WatcherThread does not
participate in).


Right. The WatcherThread intentionally does not participate in
safepoints when using the PeriodicTask_lock. To me, that means
that other threads using that lock should not participate in
the safepoint protocol.

I believe we're trying to be consistent about how we use locks
with respect to honoring the safepoint protocol or not. Markus G
has a bug fix in process where he's trying to add sanity checks
that detect inconsistent use of locks. In this particular case,
I believe that the Main thread's use of the PeriodicTask_lock
with the safepoint protocol enabled would be seen as inconsistent.
However, I may not have all the details for what Markus is doing.



I wonder if the WatcherThread should really be using
the async variant of VM_ForceSafepoint here?


The WatcherThread is trying to do a JavaThread::java_suspend()
call. JavaThread::java_suspend() cannot return to the caller
until it is sure that the target thread will not execute any
more bytecodes. Switching to an async VM_ForceSafepoint would
break that invariant and cause subtle deadlocks because a target
JavaThread could acquire a Java monitor when the suspend
requesting thread thinks it is suspended. Been down that road
before and it is not pretty.






The interesting part of this deadlock is that I think that it
is possible for other periodic tasks to hit it. Anything that
causes the WatcherThread to start a safepoint while processing
a periodic task should be susceptible to this race. Think about
the -XX:+DeoptimizeALot option and how it causes VM_Deopt
requests on thread state transitions... Interesting...


I don't think so. You need three threads involved to get the deadlock.


But that isn't the point. As you state this deadlock, at VM shutdown,
could impact any synchronous safepoint operations executed by the
WatcherThread.


Right. The problem is that the WatcherThread holds the
PeriodicTask_lock across the potential for VM operations
so we have to be very, very careful with non-WatcherThread
uses of that lock to avoid deadlocks.





That aside ...


In the current case the main thread's locking of the PeriodicTask_lock
without a safepoint check is what causes the problem

Re: RFR 8048193: [tests] Replace JPS and stdout based PID retrieval by Process.getPid()

[Jaroslav Bachorik]

On 07/01/2014 02:14 PM, Staffan Larsen wrote:


On 1 jul 2014, at 14:09, Jaroslav Bachorik  wrote:


On 07/01/2014 11:37 AM, Staffan Larsen wrote:


On 1 jul 2014, at 11:05, Jaroslav Bachorik mailto:jaroslav.bacho...@oracle.com>> wrote:


On 07/01/2014 10:54 AM, Staffan Larsen wrote:


On 1 jul 2014, at 10:29, Jaroslav Bachorik
mailto:jaroslav.bacho...@oracle.com>>
wrote:


On 07/01/2014 08:17 AM, Staffan Larsen wrote:

Jaroslav,

Great cleanup!

How about using Process.destroyForcibly() instead of sending the
“shutdown” message? Maybe not as “nice”, but much less code.


The target process needs to hang around for a while till the test
tries to shut it down. We would need to put a monitor wait there and
rely on the OS being able to shut the process down.


Not sure what you mean. Why can’t we terminate the process at the
same place we call RunnerUtil.stopApplication()?


The target application does nothing except of waiting to be shut down.
It needs to hang around for the test to be able to attach to it. It
used to listen on a socket to receive the shutdown message, now it
reads from stdin to block and wait for shutdown. If the read from
stdin is removed we would need to add another wait mechanism to make
sure the application is alive until the test shuts it down.


Right. I would suggest a simple "for(;;) Thread.sleep(1000);” loop for this.






The Process.destroyForcibly() spec leaves it to the OS specific
implementations to do the right thing. For the major OSes it seems
to force kill the process but I'm not sure about eg. embedded devices.


I think the idea of Process.destroyForcibly() is that we /can/ rely
on it. If we can’t rely on our own implementation, then the API is
not very usable.


Might be the case -
...
* The default implementation of this method invokes {@link #destroy}
* and so may not forcibly terminate the process. Concrete implementations
* of this class are strongly encouraged to override this method with a
* compliant implementation.
...

If it can be confirmed that a process will always be forcibly
destroyed I have nothing against using this API call instead of
hand-crafting the shutdown mechanism.


All of the implementation of Process in the JDK do provide methods for
forcibly terminating the process. The above reference to a default
implementation is for allowing other Process implementation outside of
the JDK. Our testing would never encounter those.


Yes, this sounds reasonable. Unfortunately, Process.destroyForcibly() sets the 
application's exit code to 143 and it makes a lot of the tests fail :(

I would better leave it at checking the stdin for the shutdown message, even 
though it requires more code.


OK. I just want us to consider using it for future tests.


Sure. Thanks!

-JB-



/Staffan



-JB-





/Staffan



-JB-







test/sun/tools/jstatd/JstatdTest.java:
323 port = Integer.toString(3);
//Utils.getFreePort());
Looks like a mistake?


Definitely a mistake :( Thanks for spotting it!

Updated webrev: http://cr.openjdk.java.net/~jbachorik/8048193/webrev.01


Looks good!

Thanks,
/Staffan




-JB-




/Staffan


On 30 jun 2014, at 18:43, Jaroslav Bachorik
mailto:jaroslav.bacho...@oracle.com>> wrote:


Please, review the following test change.

Issue : https://bugs.openjdk.java.net/browse/JDK-8048193
Webrev: http://cr.openjdk.java.net/~jbachorik/8048193/webrev.00

Intricate log parsing in order to get an application PID is
replaced with the new Process.getPid() API call. While doing this
cleanup it also become obvious that it was unnecessary to start a
socket server for each launched test application just in order to
shut it down when the same functionality can be achieved through
the usage of stdin/stdout provided by the Process instance.

Thanks,

-JB-

Re: RFR(M): 8028474: sun/jvmstat/monitor/MonitoredVm/MonitorVmStartTerminate.sh timeout, leaves looping process behind

[roger riggs]

Hi Erik,

Consider switching to System.nanoTime;  it is not sensitive to clock changes
and avoids leaving a land mine that may cause a spurious non-repeatable 
test failure.
'Deducing it from the log' means there is a failure and creates probably 
an hour or two of work
for some quality engineer and burns a couple of hours re-running the 
test run.


Roger



On 7/1/2014 3:37 AM, Erik Gahlin wrote:



JavaProcess.waitForRemoval: How about using timestamps 
(currentTimeMillis()) before the loop and for each ite

ration to determine if the timeout has expired (instead of "time+=100”)?

The code now uses currentTimeMillis(). Premature timeouts due to clock 
changes can be deduced from the log.

Re: RFR: JDK-8048710 Use ServiceLoader for the jvmstat protocols

[Staffan Larsen]

Thanks, Alan.

On 30 jun 2014, at 15:25, Alan Bateman  wrote:

> On 30/06/2014 12:22, Staffan Larsen wrote:
>> All,
>> 
>> The jvmstat implementation has three different protocols (file, local and 
>> rmi). These protocol implementations are currently loaded with 
>> Class.forName() using a specific scheme to create the class name. A more 
>> standard approach is to use ServiceLoader for this instead. This also makes 
>> it easier to break out different implementation classes for different 
>> packagings.
>> 
>> webrev: http://cr.openjdk.java.net/~sla/8048710/webrev.00/
>> bug: https://bugs.openjdk.java.net/browse/JDK-8048710
>> 
> This looks good to me.
> 
> One comment on monitoredHostServiceLoader is that it specifies the class 
> loader as null and so will search the system class loader. I think this is 
> okay as it provides a bit of flexibility to deploy other protocols on the 
> class path if needed.
> 
> One other comment is that using ServiceLoader with a security manager often 
> requires additional permissions to be granted because it involve scanning the 
> class path. I don't know if we have any tests that use jvmstat with a 
> security manager, just something to mention in case it comes up.
> 
> -Alan
> 
> 

Re: RFR 8048193: [tests] Replace JPS and stdout based PID retrieval by Process.getPid()

[Staffan Larsen]

On 1 jul 2014, at 14:09, Jaroslav Bachorik  wrote:

> On 07/01/2014 11:37 AM, Staffan Larsen wrote:
>> 
>> On 1 jul 2014, at 11:05, Jaroslav Bachorik > > wrote:
>> 
>>> On 07/01/2014 10:54 AM, Staffan Larsen wrote:
 
 On 1 jul 2014, at 10:29, Jaroslav Bachorik
 mailto:jaroslav.bacho...@oracle.com>>
 wrote:
 
> On 07/01/2014 08:17 AM, Staffan Larsen wrote:
>> Jaroslav,
>> 
>> Great cleanup!
>> 
>> How about using Process.destroyForcibly() instead of sending the
>> “shutdown” message? Maybe not as “nice”, but much less code.
> 
> The target process needs to hang around for a while till the test
> tries to shut it down. We would need to put a monitor wait there and
> rely on the OS being able to shut the process down.
 
 Not sure what you mean. Why can’t we terminate the process at the
 same place we call RunnerUtil.stopApplication()?
>>> 
>>> The target application does nothing except of waiting to be shut down.
>>> It needs to hang around for the test to be able to attach to it. It
>>> used to listen on a socket to receive the shutdown message, now it
>>> reads from stdin to block and wait for shutdown. If the read from
>>> stdin is removed we would need to add another wait mechanism to make
>>> sure the application is alive until the test shuts it down.
>> 
>> Right. I would suggest a simple "for(;;) Thread.sleep(1000);” loop for this.
>> 
>>> 
 
> The Process.destroyForcibly() spec leaves it to the OS specific
> implementations to do the right thing. For the major OSes it seems
> to force kill the process but I'm not sure about eg. embedded devices.
 
 I think the idea of Process.destroyForcibly() is that we /can/ rely
 on it. If we can’t rely on our own implementation, then the API is
 not very usable.
>>> 
>>> Might be the case -
>>> ...
>>> * The default implementation of this method invokes {@link #destroy}
>>> * and so may not forcibly terminate the process. Concrete implementations
>>> * of this class are strongly encouraged to override this method with a
>>> * compliant implementation.
>>> ...
>>> 
>>> If it can be confirmed that a process will always be forcibly
>>> destroyed I have nothing against using this API call instead of
>>> hand-crafting the shutdown mechanism.
>> 
>> All of the implementation of Process in the JDK do provide methods for
>> forcibly terminating the process. The above reference to a default
>> implementation is for allowing other Process implementation outside of
>> the JDK. Our testing would never encounter those.
> 
> Yes, this sounds reasonable. Unfortunately, Process.destroyForcibly() sets 
> the application's exit code to 143 and it makes a lot of the tests fail :(
> 
> I would better leave it at checking the stdin for the shutdown message, even 
> though it requires more code.

OK. I just want us to consider using it for future tests.

/Staffan

> 
> -JB-
> 
> 
> 
>> 
>> /Staffan
>> 
>>> 
>>> -JB-
>>> 
 
> 
>> 
>> test/sun/tools/jstatd/JstatdTest.java:
>> 323 port = Integer.toString(3);
>> //Utils.getFreePort());
>> Looks like a mistake?
> 
> Definitely a mistake :( Thanks for spotting it!
> 
> Updated webrev: http://cr.openjdk.java.net/~jbachorik/8048193/webrev.01
 
 Looks good!
 
 Thanks,
 /Staffan
 
 
> 
> -JB-
> 
>> 
>> 
>> /Staffan
>> 
>> 
>> On 30 jun 2014, at 18:43, Jaroslav Bachorik
>> > > wrote:
>> 
>>> Please, review the following test change.
>>> 
>>> Issue : https://bugs.openjdk.java.net/browse/JDK-8048193
>>> Webrev: http://cr.openjdk.java.net/~jbachorik/8048193/webrev.00
>>> 
>>> Intricate log parsing in order to get an application PID is
>>> replaced with the new Process.getPid() API call. While doing this
>>> cleanup it also become obvious that it was unnecessary to start a
>>> socket server for each launched test application just in order to
>>> shut it down when the same functionality can be achieved through
>>> the usage of stdin/stdout provided by the Process instance.
>>> 
>>> Thanks,
>>> 
>>> -JB-
>> 
> 

Re: RFR 8048193: [tests] Replace JPS and stdout based PID retrieval by Process.getPid()

[Jaroslav Bachorik]

On 07/01/2014 11:37 AM, Staffan Larsen wrote:


On 1 jul 2014, at 11:05, Jaroslav Bachorik mailto:jaroslav.bacho...@oracle.com>> wrote:


On 07/01/2014 10:54 AM, Staffan Larsen wrote:


On 1 jul 2014, at 10:29, Jaroslav Bachorik
mailto:jaroslav.bacho...@oracle.com>>
wrote:


On 07/01/2014 08:17 AM, Staffan Larsen wrote:

Jaroslav,

Great cleanup!

How about using Process.destroyForcibly() instead of sending the
“shutdown” message? Maybe not as “nice”, but much less code.


The target process needs to hang around for a while till the test
tries to shut it down. We would need to put a monitor wait there and
rely on the OS being able to shut the process down.


Not sure what you mean. Why can’t we terminate the process at the
same place we call RunnerUtil.stopApplication()?


The target application does nothing except of waiting to be shut down.
It needs to hang around for the test to be able to attach to it. It
used to listen on a socket to receive the shutdown message, now it
reads from stdin to block and wait for shutdown. If the read from
stdin is removed we would need to add another wait mechanism to make
sure the application is alive until the test shuts it down.


Right. I would suggest a simple "for(;;) Thread.sleep(1000);” loop for this.






The Process.destroyForcibly() spec leaves it to the OS specific
implementations to do the right thing. For the major OSes it seems
to force kill the process but I'm not sure about eg. embedded devices.


I think the idea of Process.destroyForcibly() is that we /can/ rely
on it. If we can’t rely on our own implementation, then the API is
not very usable.


Might be the case -
...
* The default implementation of this method invokes {@link #destroy}
* and so may not forcibly terminate the process. Concrete implementations
* of this class are strongly encouraged to override this method with a
* compliant implementation.
...

If it can be confirmed that a process will always be forcibly
destroyed I have nothing against using this API call instead of
hand-crafting the shutdown mechanism.


All of the implementation of Process in the JDK do provide methods for
forcibly terminating the process. The above reference to a default
implementation is for allowing other Process implementation outside of
the JDK. Our testing would never encounter those.


Yes, this sounds reasonable. Unfortunately, Process.destroyForcibly() 
sets the application's exit code to 143 and it makes a lot of the tests 
fail :(


I would better leave it at checking the stdin for the shutdown message, 
even though it requires more code.


-JB-





/Staffan



-JB-







test/sun/tools/jstatd/JstatdTest.java:
 323 port = Integer.toString(3);
//Utils.getFreePort());
 Looks like a mistake?


Definitely a mistake :( Thanks for spotting it!

Updated webrev: http://cr.openjdk.java.net/~jbachorik/8048193/webrev.01


Looks good!

Thanks,
/Staffan




-JB-




/Staffan


On 30 jun 2014, at 18:43, Jaroslav Bachorik
mailto:jaroslav.bacho...@oracle.com>> wrote:


Please, review the following test change.

Issue : https://bugs.openjdk.java.net/browse/JDK-8048193
Webrev: http://cr.openjdk.java.net/~jbachorik/8048193/webrev.00

Intricate log parsing in order to get an application PID is
replaced with the new Process.getPid() API call. While doing this
cleanup it also become obvious that it was unnecessary to start a
socket server for each launched test application just in order to
shut it down when the same functionality can be achieved through
the usage of stdin/stdout provided by the Process instance.

Thanks,

-JB-

Re: RFR: 8046883 com/sun/jdi/ProcessAttachTest.sh gets "java.io.IOException: Invalid process identifier" on windows

[Dmitry Samersoff]

Staffan,

Looks good for me.

-Dmitry


On 2014-07-01 15:44, Staffan Larsen wrote:
> I’m still looking for a Review of this test code.
> 
> Thanks,
> /Staffan
> 
> On 23 jun 2014, at 10:33, Staffan Larsen  > wrote:
> 
>> Fancy!
>>
>> new review: http://cr.openjdk.java.net/~sla/8046883/webrev.01/
>>
>> /Staffan
>>
>> On 18 jun 2014, at 13:59, Peter Allwin > > wrote:
>>
>>> This looks a lot better!
>>>
>>> (Since we’re using fancy new features we could use streams to find
>>> the connector instance)
>>>
>>> AttachingConnector ac =
>>> Bootstrap.virtualMachineManager().attachingConnectors()
>>> .stream()
>>> .filter(c -> c.name().equals("com.sun.jdi.ProcessAttach"))
>>> .findFirst()
>>> .orElseThrow(() -> new RuntimeException("Unable to locate
>>> ProcessAttachingConnector"));
>>>
>>> Thanks!
>>> /peter
>>>
>>> On 17 Jun 2014, at 19:46, Staffan Larsen >> > wrote:
>>>
 Here is a rewrite of the test in Java instead of a shell script.
 Should be easier to maintain.

 webrev: http://cr.openjdk.java.net/~sla/8046883/webrev.00/

 Thanks,
 /Staffan

 On 17 jun 2014, at 15:12, Staffan Larsen >>> > wrote:

>
> On 17 jun 2014, at 15:03, Alan Bateman  > wrote:
>
>> On 17/06/2014 13:35, Staffan Larsen wrote:
>>> :
>>>
>>> It could be a timing issue, but in the other direction. If cygwin
>>> hasn’t yet started the real windows process when I run ps, then
>>> maybe ps will not list it. But given the “sleep 2” before the ps
>>> invocation, the process should have had time to started. No
>>> guarantees of course.
>>>
>>> Making the sleep shorter will not help as the process we are
>>> starting will not terminate until we tell it to.
>>>
>>>
>> Okay, although what I was suggesting is to use your patch but
>> additionally move the sleep at L79 into the new while loop so that
>> it doesn't spin quickly through the 10 iterations. That would give
>> the test 10 attempts (and 10 seconds) to get the pid.
>
> Ah, I see. I misunderstood your comment.
>
> I started looking at rewriting the test in pure Java instead of the
> shell script. With the new Process.getPid() this looks like the
> best approach. I’ll come back with a new review request soon.
>
> /Staffan

>>>
>>
> 


-- 
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.

Re: RFR: 8046883 com/sun/jdi/ProcessAttachTest.sh gets "java.io.IOException: Invalid process identifier" on windows

[Staffan Larsen]

I’m still looking for a Review of this test code.

Thanks,
/Staffan

On 23 jun 2014, at 10:33, Staffan Larsen  wrote:

> Fancy!
> 
> new review: http://cr.openjdk.java.net/~sla/8046883/webrev.01/
> 
> /Staffan
> 
> On 18 jun 2014, at 13:59, Peter Allwin  wrote:
> 
>> This looks a lot better!
>> 
>> (Since we’re using fancy new features we could use streams to find the 
>> connector instance)
>> 
>> AttachingConnector ac = 
>> Bootstrap.virtualMachineManager().attachingConnectors()
>> .stream()
>> .filter(c -> c.name().equals("com.sun.jdi.ProcessAttach"))
>> .findFirst()
>> .orElseThrow(() -> new RuntimeException("Unable to locate 
>> ProcessAttachingConnector"));
>> 
>> Thanks!
>> /peter
>> 
>> On 17 Jun 2014, at 19:46, Staffan Larsen  wrote:
>> 
>>> Here is a rewrite of the test in Java instead of a shell script. Should be 
>>> easier to maintain.
>>> 
>>> webrev: http://cr.openjdk.java.net/~sla/8046883/webrev.00/
>>> 
>>> Thanks,
>>> /Staffan
>>> 
>>> On 17 jun 2014, at 15:12, Staffan Larsen  wrote:
>>> 
 
 On 17 jun 2014, at 15:03, Alan Bateman  wrote:
 
> On 17/06/2014 13:35, Staffan Larsen wrote:
>> :
>> 
>> It could be a timing issue, but in the other direction. If cygwin hasn’t 
>> yet started the real windows process when I run ps, then maybe ps will 
>> not list it. But given the “sleep 2” before the ps invocation, the 
>> process should have had time to started. No guarantees of course.
>> 
>> Making the sleep shorter will not help as the process we are starting 
>> will not terminate until we tell it to.
>> 
>> 
> Okay, although what I was suggesting is to use your patch but 
> additionally move the sleep at L79 into the new while loop so that it 
> doesn't spin quickly through the 10 iterations. That would give the test 
> 10 attempts (and 10 seconds) to get the pid.
 
 Ah, I see. I misunderstood your comment.
 
 I started looking at rewriting the test in pure Java instead of the shell 
 script. With the new Process.getPid() this looks like the best approach. 
 I’ll come back with a new review request soon.
 
 /Staffan
>>> 
>> 
> 

Re: RFR 8048193: [tests] Replace JPS and stdout based PID retrieval by Process.getPid()

[Staffan Larsen]

On 1 jul 2014, at 11:05, Jaroslav Bachorik  wrote:

> On 07/01/2014 10:54 AM, Staffan Larsen wrote:
>> 
>> On 1 jul 2014, at 10:29, Jaroslav Bachorik  
>> wrote:
>> 
>>> On 07/01/2014 08:17 AM, Staffan Larsen wrote:
 Jaroslav,
 
 Great cleanup!
 
 How about using Process.destroyForcibly() instead of sending the 
 “shutdown” message? Maybe not as “nice”, but much less code.
>>> 
>>> The target process needs to hang around for a while till the test tries to 
>>> shut it down. We would need to put a monitor wait there and rely on the OS 
>>> being able to shut the process down.
>> 
>> Not sure what you mean. Why can’t we terminate the process at the same place 
>> we call RunnerUtil.stopApplication()?
> 
> The target application does nothing except of waiting to be shut down. It 
> needs to hang around for the test to be able to attach to it. It used to 
> listen on a socket to receive the shutdown message, now it reads from stdin 
> to block and wait for shutdown. If the read from stdin is removed we would 
> need to add another wait mechanism to make sure the application is alive 
> until the test shuts it down.

Right. I would suggest a simple "for(;;) Thread.sleep(1000);” loop for this.

> 
>> 
>>> The Process.destroyForcibly() spec leaves it to the OS specific 
>>> implementations to do the right thing. For the major OSes it seems to force 
>>> kill the process but I'm not sure about eg. embedded devices.
>> 
>> I think the idea of Process.destroyForcibly() is that we /can/ rely on it. 
>> If we can’t rely on our own implementation, then the API is not very usable.
> 
> Might be the case -
> ...
> * The default implementation of this method invokes {@link #destroy}
> * and so may not forcibly terminate the process. Concrete implementations
> * of this class are strongly encouraged to override this method with a
> * compliant implementation.
> ...
> 
> If it can be confirmed that a process will always be forcibly destroyed I 
> have nothing against using this API call instead of hand-crafting the 
> shutdown mechanism.

All of the implementation of Process in the JDK do provide methods for forcibly 
terminating the process. The above reference to a default implementation is for 
allowing other Process implementation outside of the JDK. Our testing would 
never encounter those.

/Staffan

> 
> -JB-
> 
>> 
>>> 
 
 test/sun/tools/jstatd/JstatdTest.java:
  323 port = Integer.toString(3); 
 //Utils.getFreePort());
  Looks like a mistake?
>>> 
>>> Definitely a mistake :( Thanks for spotting it!
>>> 
>>> Updated webrev: http://cr.openjdk.java.net/~jbachorik/8048193/webrev.01
>> 
>> Looks good!
>> 
>> Thanks,
>> /Staffan
>> 
>> 
>>> 
>>> -JB-
>>> 
 
 
 /Staffan
 
 
 On 30 jun 2014, at 18:43, Jaroslav Bachorik  
 wrote:
 
> Please, review the following test change.
> 
> Issue : https://bugs.openjdk.java.net/browse/JDK-8048193
> Webrev: http://cr.openjdk.java.net/~jbachorik/8048193/webrev.00
> 
> Intricate log parsing in order to get an application PID is replaced with 
> the new Process.getPid() API call. While doing this cleanup it also 
> become obvious that it was unnecessary to start a socket server for each 
> launched test application just in order to shut it down when the same 
> functionality can be achieved through the usage of stdin/stdout provided 
> by the Process instance.
> 
> Thanks,
> 
> -JB-

Re: RFR 8048193: [tests] Replace JPS and stdout based PID retrieval by Process.getPid()

[Jaroslav Bachorik]

On 07/01/2014 10:54 AM, Staffan Larsen wrote:


On 1 jul 2014, at 10:29, Jaroslav Bachorik  wrote:


On 07/01/2014 08:17 AM, Staffan Larsen wrote:

Jaroslav,

Great cleanup!

How about using Process.destroyForcibly() instead of sending the “shutdown” 
message? Maybe not as “nice”, but much less code.


The target process needs to hang around for a while till the test tries to shut 
it down. We would need to put a monitor wait there and rely on the OS being 
able to shut the process down.


Not sure what you mean. Why can’t we terminate the process at the same place we 
call RunnerUtil.stopApplication()?


The target application does nothing except of waiting to be shut down. 
It needs to hang around for the test to be able to attach to it. It used 
to listen on a socket to receive the shutdown message, now it reads from 
stdin to block and wait for shutdown. If the read from stdin is removed 
we would need to add another wait mechanism to make sure the application 
is alive until the test shuts it down.





The Process.destroyForcibly() spec leaves it to the OS specific implementations 
to do the right thing. For the major OSes it seems to force kill the process 
but I'm not sure about eg. embedded devices.


I think the idea of Process.destroyForcibly() is that we /can/ rely on it. If 
we can’t rely on our own implementation, then the API is not very usable.


Might be the case -
...
* The default implementation of this method invokes {@link #destroy}
* and so may not forcibly terminate the process. Concrete implementations
* of this class are strongly encouraged to override this method with a
* compliant implementation.
...

If it can be confirmed that a process will always be forcibly destroyed 
I have nothing against using this API call instead of hand-crafting the 
shutdown mechanism.


-JB-







test/sun/tools/jstatd/JstatdTest.java:
  323 port = Integer.toString(3); 
//Utils.getFreePort());
  Looks like a mistake?


Definitely a mistake :( Thanks for spotting it!

Updated webrev: http://cr.openjdk.java.net/~jbachorik/8048193/webrev.01


Looks good!

Thanks,
/Staffan




-JB-




/Staffan


On 30 jun 2014, at 18:43, Jaroslav Bachorik  
wrote:


Please, review the following test change.

Issue : https://bugs.openjdk.java.net/browse/JDK-8048193
Webrev: http://cr.openjdk.java.net/~jbachorik/8048193/webrev.00

Intricate log parsing in order to get an application PID is replaced with the 
new Process.getPid() API call. While doing this cleanup it also become obvious 
that it was unnecessary to start a socket server for each launched test 
application just in order to shut it down when the same functionality can be 
achieved through the usage of stdin/stdout provided by the Process instance.

Thanks,

-JB-

Re: RFR 8048193: [tests] Replace JPS and stdout based PID retrieval by Process.getPid()

[David Holmes]

On 1/07/2014 6:54 PM, Staffan Larsen wrote:


On 1 jul 2014, at 10:29, Jaroslav Bachorik  wrote:


On 07/01/2014 08:17 AM, Staffan Larsen wrote:

Jaroslav,

Great cleanup!

How about using Process.destroyForcibly() instead of sending the “shutdown” 
message? Maybe not as “nice”, but much less code.


The target process needs to hang around for a while till the test tries to shut 
it down. We would need to put a monitor wait there and rely on the OS being 
able to shut the process down.


Not sure what you mean. Why can’t we terminate the process at the same place we 
call RunnerUtil.stopApplication()?


The Process.destroyForcibly() spec leaves it to the OS specific implementations 
to do the right thing. For the major OSes it seems to force kill the process 
but I'm not sure about eg. embedded devices.


I think the idea of Process.destroyForcibly() is that we /can/ rely on it. If 
we can’t rely on our own implementation, then the API is not very usable.


Right. It is the OS that is key not the device. Linux/Solaris will send 
a SIGKILL. Windows does terminateProcess for both destroy and 
destroyForcibly.


David
-






test/sun/tools/jstatd/JstatdTest.java:
  323 port = Integer.toString(3); 
//Utils.getFreePort());
  Looks like a mistake?


Definitely a mistake :( Thanks for spotting it!

Updated webrev: http://cr.openjdk.java.net/~jbachorik/8048193/webrev.01


Looks good!

Thanks,
/Staffan




-JB-




/Staffan


On 30 jun 2014, at 18:43, Jaroslav Bachorik  
wrote:


Please, review the following test change.

Issue : https://bugs.openjdk.java.net/browse/JDK-8048193
Webrev: http://cr.openjdk.java.net/~jbachorik/8048193/webrev.00

Intricate log parsing in order to get an application PID is replaced with the 
new Process.getPid() API call. While doing this cleanup it also become obvious 
that it was unnecessary to start a socket server for each launched test 
application just in order to shut it down when the same functionality can be 
achieved through the usage of stdin/stdout provided by the Process instance.

Thanks,

-JB-

Re: RFR 8048193: [tests] Replace JPS and stdout based PID retrieval by Process.getPid()

[Staffan Larsen]

On 1 jul 2014, at 10:29, Jaroslav Bachorik  wrote:

> On 07/01/2014 08:17 AM, Staffan Larsen wrote:
>> Jaroslav,
>> 
>> Great cleanup!
>> 
>> How about using Process.destroyForcibly() instead of sending the “shutdown” 
>> message? Maybe not as “nice”, but much less code.
> 
> The target process needs to hang around for a while till the test tries to 
> shut it down. We would need to put a monitor wait there and rely on the OS 
> being able to shut the process down.

Not sure what you mean. Why can’t we terminate the process at the same place we 
call RunnerUtil.stopApplication()?

> The Process.destroyForcibly() spec leaves it to the OS specific 
> implementations to do the right thing. For the major OSes it seems to force 
> kill the process but I'm not sure about eg. embedded devices.

I think the idea of Process.destroyForcibly() is that we /can/ rely on it. If 
we can’t rely on our own implementation, then the API is not very usable.

> 
>> 
>> test/sun/tools/jstatd/JstatdTest.java:
>>  323 port = Integer.toString(3); 
>> //Utils.getFreePort());
>>  Looks like a mistake?
> 
> Definitely a mistake :( Thanks for spotting it!
> 
> Updated webrev: http://cr.openjdk.java.net/~jbachorik/8048193/webrev.01

Looks good!

Thanks,
/Staffan


> 
> -JB-
> 
>> 
>> 
>> /Staffan
>> 
>> 
>> On 30 jun 2014, at 18:43, Jaroslav Bachorik  
>> wrote:
>> 
>>> Please, review the following test change.
>>> 
>>> Issue : https://bugs.openjdk.java.net/browse/JDK-8048193
>>> Webrev: http://cr.openjdk.java.net/~jbachorik/8048193/webrev.00
>>> 
>>> Intricate log parsing in order to get an application PID is replaced with 
>>> the new Process.getPid() API call. While doing this cleanup it also become 
>>> obvious that it was unnecessary to start a socket server for each launched 
>>> test application just in order to shut it down when the same functionality 
>>> can be achieved through the usage of stdin/stdout provided by the Process 
>>> instance.
>>> 
>>> Thanks,
>>> 
>>> -JB-
>> 
> 

Re: RFR: 8048687 [TESTBUG] com/sun/jdi/ExclusiveBind.java "Could not find or load main class"

[David Holmes]

Looks good to me too!

David

On 1/07/2014 5:38 PM, Staffan Larsen wrote:

Please review this test fix.

webrev: http://cr.openjdk.java.net/~sla/8048687/webrev.00/
bug: https://bugs.openjdk.java.net/browse/JDK-8048687

The test tries to start a process but fails with:


--System.out:(35/1961)--
vmOpts:
javaOpts: -Xmixed -server -XX:MaxRAMFraction=8 -XX:+CreateMinidumpOnCrash 
-XX:ReservedCodeCacheSize=256M
Command line: 
[/export/local/aurora/sandbox/sca/vmsqe/jdk/nightly/fastdebug/rt_baseline/macosx-amd64/bin/java
 -classpath /export/local/aurora/sandbox/results/workDir/classes/1/com/sun/jdi  
-Xmixed -server -XX:MaxRAMFraction=8 -XX:+CreateMinidumpOnCrash 
-XX:ReservedCodeCacheSize=256M 
-agentlib:jdwp=transport=dt_socket,server=y,suspend=y,address=61682 HelloWorld ]
...
--System.err:(16/856)--
[process1] Error: Could not find or load main class
Failed to start a process (thread dump follows)


At first glance the command line looks ok, but what I missed here is that there are two 
spaces before "-Xmixed". Two spaces in a command line is ok, but in this case 
it means that the String[] argument to ProcessBuilder has one empty String element. That 
is not ok since that would turn into an actual empty argument to the process. We can 
actually see this in the output from the process, too:

   [process1] Error: Could not find or load main class

If it was the HelloWorld class that could not be found it should have said:

   [process1] Error: Could not find or load main class HelloWorld

But now it is the empty string class that we can't find.

The fix makes sure there are no empty Strings in the process arguments.

Thanks,
/Staffan

Re: RFR 8048193: [tests] Replace JPS and stdout based PID retrieval by Process.getPid()

[Jaroslav Bachorik]

On 07/01/2014 08:17 AM, Staffan Larsen wrote:

Jaroslav,

Great cleanup!

How about using Process.destroyForcibly() instead of sending the “shutdown” 
message? Maybe not as “nice”, but much less code.


The target process needs to hang around for a while till the test tries 
to shut it down. We would need to put a monitor wait there and rely on 
the OS being able to shut the process down. The 
Process.destroyForcibly() spec leaves it to the OS specific 
implementations to do the right thing. For the major OSes it seems to 
force kill the process but I'm not sure about eg. embedded devices.




test/sun/tools/jstatd/JstatdTest.java:
  323 port = Integer.toString(3); 
//Utils.getFreePort());
  Looks like a mistake?


Definitely a mistake :( Thanks for spotting it!

Updated webrev: http://cr.openjdk.java.net/~jbachorik/8048193/webrev.01

-JB-




/Staffan


On 30 jun 2014, at 18:43, Jaroslav Bachorik  
wrote:


Please, review the following test change.

Issue : https://bugs.openjdk.java.net/browse/JDK-8048193
Webrev: http://cr.openjdk.java.net/~jbachorik/8048193/webrev.00

Intricate log parsing in order to get an application PID is replaced with the 
new Process.getPid() API call. While doing this cleanup it also become obvious 
that it was unnecessary to start a socket server for each launched test 
application just in order to shut it down when the same functionality can be 
achieved through the usage of stdin/stdout provided by the Process instance.

Thanks,

-JB-

Re: RFR: 8048687 [TESTBUG] com/sun/jdi/ExclusiveBind.java "Could not find or load main class"

[Staffan Larsen]

Erik, Jaroslav: Thanks!

I need one official Reviewer to have a look as well.

Thanks,
/Staffan


On 1 jul 2014, at 10:23, Erik Gahlin  wrote:

> Looks good!
> 
> Erik
> 
> Staffan Larsen skrev 2014-07-01 09:38:
>> Please review this test fix.
>> 
>> webrev: http://cr.openjdk.java.net/~sla/8048687/webrev.00/
>> bug: https://bugs.openjdk.java.net/browse/JDK-8048687
>> 
>> The test tries to start a process but fails with:
>> 
>> 
>> --System.out:(35/1961)--
>> vmOpts:
>> javaOpts: -Xmixed -server -XX:MaxRAMFraction=8 -XX:+CreateMinidumpOnCrash 
>> -XX:ReservedCodeCacheSize=256M
>> Command line: 
>> [/export/local/aurora/sandbox/sca/vmsqe/jdk/nightly/fastdebug/rt_baseline/macosx-amd64/bin/java
>>  -classpath 
>> /export/local/aurora/sandbox/results/workDir/classes/1/com/sun/jdi  -Xmixed 
>> -server -XX:MaxRAMFraction=8 -XX:+CreateMinidumpOnCrash 
>> -XX:ReservedCodeCacheSize=256M 
>> -agentlib:jdwp=transport=dt_socket,server=y,suspend=y,address=61682 
>> HelloWorld ]
>> ...
>> --System.err:(16/856)--
>> [process1] Error: Could not find or load main class
>> Failed to start a process (thread dump follows)
>> 
>> 
>> At first glance the command line looks ok, but what I missed here is that 
>> there are two spaces before "-Xmixed". Two spaces in a command line is ok, 
>> but in this case it means that the String[] argument to ProcessBuilder has 
>> one empty String element. That is not ok since that would turn into an 
>> actual empty argument to the process. We can actually see this in the output 
>> from the process, too:
>> 
>>   [process1] Error: Could not find or load main class
>> 
>> If it was the HelloWorld class that could not be found it should have said:
>> 
>>   [process1] Error: Could not find or load main class HelloWorld
>> 
>> But now it is the empty string class that we can't find.
>> 
>> The fix makes sure there are no empty Strings in the process arguments.
>> 
>> Thanks,
>> /Staffan
> 

Re: RFR: 8048687 [TESTBUG] com/sun/jdi/ExclusiveBind.java "Could not find or load main class"

[Erik Gahlin]

Looks good!

Erik

Staffan Larsen skrev 2014-07-01 09:38:

Please review this test fix.

webrev: http://cr.openjdk.java.net/~sla/8048687/webrev.00/
bug: https://bugs.openjdk.java.net/browse/JDK-8048687

The test tries to start a process but fails with:


--System.out:(35/1961)--
vmOpts:
javaOpts: -Xmixed -server -XX:MaxRAMFraction=8 -XX:+CreateMinidumpOnCrash 
-XX:ReservedCodeCacheSize=256M
Command line: 
[/export/local/aurora/sandbox/sca/vmsqe/jdk/nightly/fastdebug/rt_baseline/macosx-amd64/bin/java
 -classpath /export/local/aurora/sandbox/results/workDir/classes/1/com/sun/jdi  
-Xmixed -server -XX:MaxRAMFraction=8 -XX:+CreateMinidumpOnCrash 
-XX:ReservedCodeCacheSize=256M 
-agentlib:jdwp=transport=dt_socket,server=y,suspend=y,address=61682 HelloWorld ]
...
--System.err:(16/856)--
[process1] Error: Could not find or load main class
Failed to start a process (thread dump follows)


At first glance the command line looks ok, but what I missed here is that there are two 
spaces before "-Xmixed". Two spaces in a command line is ok, but in this case 
it means that the String[] argument to ProcessBuilder has one empty String element. That 
is not ok since that would turn into an actual empty argument to the process. We can 
actually see this in the output from the process, too:

   [process1] Error: Could not find or load main class

If it was the HelloWorld class that could not be found it should have said:

   [process1] Error: Could not find or load main class HelloWorld

But now it is the empty string class that we can't find.

The fix makes sure there are no empty Strings in the process arguments.

Thanks,
/Staffan

Re: RFR: 8048687 [TESTBUG] com/sun/jdi/ExclusiveBind.java "Could not find or load main class"

[Jaroslav Bachorik]

Looks good!

-JB-

On 07/01/2014 09:38 AM, Staffan Larsen wrote:

Please review this test fix.

webrev: http://cr.openjdk.java.net/~sla/8048687/webrev.00/
bug: https://bugs.openjdk.java.net/browse/JDK-8048687

The test tries to start a process but fails with:


--System.out:(35/1961)--
vmOpts:
javaOpts: -Xmixed -server -XX:MaxRAMFraction=8 -XX:+CreateMinidumpOnCrash 
-XX:ReservedCodeCacheSize=256M
Command line: 
[/export/local/aurora/sandbox/sca/vmsqe/jdk/nightly/fastdebug/rt_baseline/macosx-amd64/bin/java
 -classpath /export/local/aurora/sandbox/results/workDir/classes/1/com/sun/jdi  
-Xmixed -server -XX:MaxRAMFraction=8 -XX:+CreateMinidumpOnCrash 
-XX:ReservedCodeCacheSize=256M 
-agentlib:jdwp=transport=dt_socket,server=y,suspend=y,address=61682 HelloWorld ]
...
--System.err:(16/856)--
[process1] Error: Could not find or load main class
Failed to start a process (thread dump follows)


At first glance the command line looks ok, but what I missed here is that there are two 
spaces before "-Xmixed". Two spaces in a command line is ok, but in this case 
it means that the String[] argument to ProcessBuilder has one empty String element. That 
is not ok since that would turn into an actual empty argument to the process. We can 
actually see this in the output from the process, too:

   [process1] Error: Could not find or load main class

If it was the HelloWorld class that could not be found it should have said:

   [process1] Error: Could not find or load main class HelloWorld

But now it is the empty string class that we can't find.

The fix makes sure there are no empty Strings in the process arguments.

Thanks,
/Staffan

Re: RFR(M): 8028474: sun/jvmstat/monitor/MonitoredVm/MonitorVmStartTerminate.sh timeout, leaves looping process behind

[Staffan Larsen]

Looks good!

Thanks,
/Staffan

On 1 jul 2014, at 09:37, Erik Gahlin  wrote:

> Updated webrev:
> http://cr.openjdk.java.net/~egahlin/8028474_5/
> 
> See comments inline.
> 
> Staffan Larsen skrev 2014-06-26 13:22:
>> Indentation around JavaProcess.getId() is weird.
>> 
> Fixed
>> JavaProcess.getPid/setPid/pid do not appear to be used.
>> 
> Fixed
> 
>> JavaProcess.waitForRemoval: How about using timestamps (currentTimeMillis()) 
>> before the loop and for each ite
>> ration to determine if the timeout has expired (instead of "time+=100”)?
>> 
> The code now uses currentTimeMillis(). Premature timeouts due to clock 
> changes can be deduced from the log.
> 
> I also cleaned up the log for releaseStarted/releaseTerminated
> 
>> nit: missing empty line before line 139, method releaseStarted().
>> 
> Fixed
> 
> Thanks
> Erik 
> 
>> /Staffan
>> 
>> 
>> On 26 jun 2014, at 00:56, Erik Gahlin  wrote:
>> 
>>> It didn't work. 
>>> 
>>> There's no termination notification, if I use Process#destroyForcibly(). I 
>>> believe the hsperfdata file is left behind so it will not be able to detect 
>>> that the process has died.
>>> 
>>> Here is an updated version that renames some variable/methods.
>>> 
>>> http://cr.openjdk.java.net/~egahlin/8028474_3/
>>> 
>>> Thanks
>>> Erik
>>> 
>>> Erik Gahlin skrev 2014-06-18 09:37:
 Didn't know about destroyForcibly(). I could try that.
 
 Erik
 
 Staffan Larsen skrev 18/06/14 09:26:
> Erik,
> 
> How about using Process.destroyForcibly() as a means to terminate the 
> process instead of using files for signaling?
> 
> /Staffan
>  
> On 17 jun 2014, at 23:13, Erik Gahlin  wrote:
> 
>> Yes, very weird
>> 
>> Could have been that I needed the tools.jar in the child processes, or 
>> it could have been something else. If I remember correctly, I got a CNF 
>> that I didn't get with the shell script, but it was few months back.
>> 
>> Anyway, I retried with JPRT and now it works without the shell script.
>> 
>> http://cr.openjdk.java.net/~egahlin/8028474_2/
>> 
>> Erik
>> 
>> Staffan Larsen skrev 2014-06-16 13:49:
>>> 
>>> On 16 jun 2014, at 12:32, Erik Gahlin  wrote:
>>> 
 Yekaterina Kantserova skrev 13/06/14 12:59:
> Erik,
> 
> is there some reason why we need to keep MonitorVmStartTerminate.sh? 
> I've moved the JTreg header to MonitorVmStartTerminate.java
 Hi Katja,
 
 That's how I did the test initially, and it works locally, but I could 
 never get it to work in JPRT without the shell script. I believe the 
 tools.jar is not on the class path.
>>> 
>>> That is weird. I see other tests that depend in tools.jar and they work 
>>> fine.
>>> 
>>> /Staffan
>>> 
>>> 
 
 Erik
> 
> /*
>  * @test
>  * @bug 4990825
>  * @summary attach to external but local JVM processes
>  * @library /lib/testlibrary
>  * @build jdk.testlibrary.*
>  * @run main MonitorVmStartTerminate
>  */
> 
> and the test works just fine.
> 
> The JTreg run contains all pathes and system properties 
> MonitorVmStartTerminate.sh tries to construct:
> ${JAVA} ${TESTVMOPTS} -Dtest.jdk=${TESTJAVA} 
> -Dtest.classes=${TESTCLASSES} -classpath ${CP} MonitorVmStartTerminate
> 
> See the log attached.
> 
> Note @build jdk.testlibrary.* instead of @build 
> jdk.testlibrary.ProcessTools to make sure all testlibrary classes are 
> compiled 
> to the right place when running tests concurrently.
> 
> Thanks,
> Katja (Not a Reviewer)
> 
> 
> 
> On 06/12/2014 12:37 AM, Erik Gahlin wrote:
>> Hi, 
>> 
>> Could I have a review of a test that has been failing 
>> intermittently. The test now uses files for synchronization 
>> instead of waiting for a process that sleeps. 
>> 
>> Webrev: 
>> http://cr.openjdk.java.net/~egahlin/8028474/ 
>> 
>> Bug: 
>> https://bugs.openjdk.java.net/browse/JDK-8028474 
>> 
>> Description: 
>> 
>> The test starts ten Java processes, each with a unique id. 
>> 
>>  Each process creates a file named after the id and then it waits 
>> for 
>>  the test to remove the file, at which the Java process exits. 
>> 
>>  The processes are monitored by the test to make sure notifications 
>>  are sent when processes are started/terminated. 
>> 
>>  To avoid Java processes being left behind, in case of an unexpected 
>>  failure, shutdown hooks are registered that remove files when the 
>> test 
>>  exits. If files are not removed, i.e. d

RFR: 8048687 [TESTBUG] com/sun/jdi/ExclusiveBind.java "Could not find or load main class"

[Staffan Larsen]

Please review this test fix.

webrev: http://cr.openjdk.java.net/~sla/8048687/webrev.00/
bug: https://bugs.openjdk.java.net/browse/JDK-8048687

The test tries to start a process but fails with:


--System.out:(35/1961)--
vmOpts: 
javaOpts: -Xmixed -server -XX:MaxRAMFraction=8 -XX:+CreateMinidumpOnCrash 
-XX:ReservedCodeCacheSize=256M
Command line: 
[/export/local/aurora/sandbox/sca/vmsqe/jdk/nightly/fastdebug/rt_baseline/macosx-amd64/bin/java
 -classpath /export/local/aurora/sandbox/results/workDir/classes/1/com/sun/jdi  
-Xmixed -server -XX:MaxRAMFraction=8 -XX:+CreateMinidumpOnCrash 
-XX:ReservedCodeCacheSize=256M 
-agentlib:jdwp=transport=dt_socket,server=y,suspend=y,address=61682 HelloWorld ]
...
--System.err:(16/856)-- 
[process1] Error: Could not find or load main class 
Failed to start a process (thread dump follows) 


At first glance the command line looks ok, but what I missed here is that there 
are two spaces before "-Xmixed". Two spaces in a command line is ok, but in 
this case it means that the String[] argument to ProcessBuilder has one empty 
String element. That is not ok since that would turn into an actual empty 
argument to the process. We can actually see this in the output from the 
process, too:

  [process1] Error: Could not find or load main class 

If it was the HelloWorld class that could not be found it should have said:

  [process1] Error: Could not find or load main class HelloWorld

But now it is the empty string class that we can't find.

The fix makes sure there are no empty Strings in the process arguments.

Thanks,
/Staffan

Re: RFR(M): 8028474: sun/jvmstat/monitor/MonitoredVm/MonitorVmStartTerminate.sh timeout, leaves looping process behind

[Erik Gahlin]

Updated webrev:
http://cr.openjdk.java.net/~egahlin/8028474_5/

See comments inline.

Staffan Larsen skrev 2014-06-26 13:22:

Indentation around JavaProcess.getId() is weird.


Fixed

JavaProcess.getPid/setPid/pid do not appear to be used.


Fixed

JavaProcess.waitForRemoval: How about using timestamps 
(currentTimeMillis()) before the loop and for each ite

ration to determine if the timeout has expired (instead of "time+=100”)?

The code now uses currentTimeMillis(). Premature timeouts due to clock 
changes can be deduced from the log.


I also cleaned up the log for releaseStarted/releaseTerminated


nit: missing empty line before line 139, method releaseStarted().


Fixed

Thanks
Erik


/Staffan


On 26 jun 2014, at 00:56, Erik Gahlin > wrote:



It didn't work.

There's no termination notification, if I use 
Process#destroyForcibly(). I believe the hsperfdata file is left 
behind so it will not be able to detect that the process has died.


Here is an updated version that renames some variable/methods.

http://cr.openjdk.java.net/~egahlin/8028474_3/

Thanks
Erik

Erik Gahlin skrev 2014-06-18 09:37:

Didn't know about destroyForcibly(). I could try that.

Erik

Staffan Larsen skrev 18/06/14 09:26:

Erik,

How about using Process.destroyForcibly() as a means to terminate 
the process instead of using files for signaling?


/Staffan

On 17 jun 2014, at 23:13, Erik Gahlin > wrote:



Yes, very weird

Could have been that I needed the tools.jar in the child 
processes, or it could have been something else. If I remember 
correctly, I got a CNF that I didn't get with the shell script, 
but it was few months back.


Anyway, I retried with JPRT and now it works without the shell script.

http://cr.openjdk.java.net/~egahlin/8028474_2/

Erik

Staffan Larsen skrev 2014-06-16 13:49:


On 16 jun 2014, at 12:32, Erik Gahlin > wrote:



Yekaterina Kantserova skrev 13/06/14 12:59:

Erik,

is there some reason why we need to keep 
MonitorVmStartTerminate.sh? I've moved the JTreg header to 
MonitorVmStartTerminate.java

Hi Katja,

That's how I did the test initially, and it works locally, but I 
could never get it to work in JPRT without the shell script. I 
believe the tools.jar is not on the class path.


That is weird. I see other tests that depend in tools.jar and 
they work fine.


/Staffan




Erik


/*
 * @test
 * @bug 4990825
 * @summary attach to external but local JVM processes
 * @library /lib/testlibrary
 * @build jdk.testlibrary.*
 * @run main MonitorVmStartTerminate
 */

and the test works just fine.

The JTreg run contains all pathes and system properties 
MonitorVmStartTerminate.sh tries to construct:
${JAVA} ${TESTVMOPTS} -Dtest.jdk=${TESTJAVA} 
-Dtest.classes=${TESTCLASSES} -classpath ${CP} 
MonitorVmStartTerminate


See the log attached.

Note *@build jdk.testlibrary.** instead of *@build 
jdk.testlibrary.ProcessTools* to make sure all testlibrary 
classes are compiled

to the right place when running tests concurrently.

Thanks,
Katja (Not a Reviewer)



On 06/12/2014 12:37 AM, Erik Gahlin wrote:

Hi,

Could I have a review of a test that has been failing
intermittently. The test now uses files for synchronization
instead of waiting for a process that sleeps.

Webrev:
http://cr.openjdk.java.net/~egahlin/8028474/

Bug:
https://bugs.openjdk.java.net/browse/JDK-8028474

Description:

The test starts ten Java processes, each with a unique id.

 Each process creates a file named after the id and then it 
waits for

 the test to remove the file, at which the Java process exits.

 The processes are monitored by the test to make sure 
notifications

 are sent when processes are started/terminated.

 To avoid Java processes being left behind, in case of an 
unexpected
 failure, shutdown hooks are registered that remove files when 
the test

 exits. If files are not removed, i.e. due to a JVM crash,
 the Java processes will exit themselves after 1000 s.

Thanks
Erik