Hi Igor,
I think it might be best to the interrupt() call out. I wanted to
see what would happen if we ever got an InterruptedException, so I
added the following to the start of Platform.shouldSAAttach():
try {
throw new InterruptedException();
} catch (InterruptedException e) {
Thread.currentThread().interrupt();
throw new RuntimeException(e);
}
At the start of the test run, before any tests are actually run, I
see the following:
failed to get value for vm.hasSAandCanAttach
java.lang.RuntimeException: java.lang.InterruptedException
at jdk.test.lib.Platform.shouldSAAttach(Platform.java:300)
at requires.VMProps.vmHasSAandCanAttach(VMProps.java:327)
at requires.VMProps$SafeMap.put(VMProps.java:69)
at requires.VMProps.call(VMProps.java:101)
at requires.VMProps.call(VMProps.java:57)
at
com.sun.javatest.regtest.agent.GetJDKProperties.run(GetJDKProperties.java:80)
at
com.sun.javatest.regtest.agent.GetJDKProperties.main(GetJDKProperties.java:54)
Caused by: java.lang.InterruptedException
at jdk.test.lib.Platform.shouldSAAttach(Platform.java:297)
... 6 more
This seems reasonable.
For each test that checks vm.hasSAandCanAttach I also see.
TEST RESULT: Error. Error evaluating expression:
vm.hasSAandCanAttach: java.lang.RuntimeException:
java.lang.InterruptedException
This too seems reasonable.
For tests that don't check vm.hasSAandCanAttach, but instead make a
runtime check that calls Platform.shouldSAAttach(), the test fails
with:
java.lang.IllegalThreadStateException: process hasn't exited
at
java.base/java.lang.ProcessImpl.exitValue(ProcessImpl.java:500)
at jdk.test.lib.apps.LingeredApp.stopApp(LingeredApp.java:380)
at jdk.test.lib.apps.LingeredApp.stopApp(LingeredApp.java:433)
at ClhsdbAttach.main(ClhsdbAttach.java:77)
at
java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native
Method)
at
java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at
java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.base/java.lang.reflect.Method.invoke(Method.java:564)
at
com.sun.javatest.regtest.agent.MainWrapper$MainThread.run(MainWrapper.java:127)
at java.base/java.lang.Thread.run(Thread.java:832)
This is a confusing way to fail. The reason it fails this way is
because stopApp() first calls waitAppTerminiate(), which does the
following:
public void waitAppTerminate() {
// This code is modeled after tail end of
ProcessTools.getOutput().
try {
appProcess.waitFor();
outPumperThread.join();
errPumperThread.join();
} catch (InterruptedException e) {
Thread.currentThread().interrupt();
// pass
}
}
I added an e.printStackTrace() call and see the following:
java.lang.InterruptedException
at java.base/java.lang.Object.wait(Native Method)
at java.base/java.lang.Object.wait(Object.java:321)
at java.base/java.lang.ProcessImpl.waitFor(ProcessImpl.java:474)
at
jdk.test.lib.apps.LingeredApp.waitAppTerminate(LingeredApp.java:239)
at jdk.test.lib.apps.LingeredApp.stopApp(LingeredApp.java:380)
at jdk.test.lib.apps.LingeredApp.stopApp(LingeredApp.java:434)
So the earlier call to interrupt() is resulting in
waitAppTerminate() not actually waiting for exit. This then results
in stopApp() getting IllegalThreadStateException when calling
Process.exitValue().
If I comment out the call to interrupt() in
Platform.shouldSAAttach(), I think the failure stack trace is much
better:
java.lang.RuntimeException: Test ERROR java.lang.RuntimeException:
java.lang.InterruptedException
at ClhsdbAttach.main(ClhsdbAttach.java:75)
at
java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native
Method)
at
java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at
java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.base/java.lang.reflect.Method.invoke(Method.java:564)
at
com.sun.javatest.regtest.agent.MainWrapper$MainThread.run(MainWrapper.java:127)
at java.base/java.lang.Thread.run(Thread.java:832)
Caused by: java.lang.RuntimeException: java.lang.InterruptedException
at jdk.test.lib.Platform.shouldSAAttach(Platform.java:300)
at ClhsdbLauncher.run(ClhsdbLauncher.java:199)
at ClhsdbAttach.main(ClhsdbAttach.java:71)
... 6 more
Caused by: java.lang.InterruptedException
at jdk.test.lib.Platform.shouldSAAttach(Platform.java:297)
... 8 more
There's still a minor issue with rethrowing the RuntimeException
encapsulated inside another RuntimeException. That the fault of the
test which is catching all Exceptions and encapsulating them in a
RuntimeException, even if the Exceptions itself is already a
RuntimeException. It should add have a catch clause for
RuntimeException, and just rethrow it without encapulating it. All
the Clhsdb tests seem to do this, so that's about 20 places to fix.
Probably not worth doing unless some other cleanup is being done at
the same time.
Chris
On 2/11/20 10:30 PM, Igor Ignatyev wrote:
I'd say yes, it's better to still call Thread::interrupt.
-- Igor
On Feb 11, 2020, at 10:19 PM, Chris Plummer
<chris.plum...@oracle.com <mailto:chris.plum...@oracle.com>> wrote:
Ok. Should I still call interrupt()?
Chris
On 2/11/20 10:07 PM, Igor Ignatyev wrote:
Hi Chris,
that's a common practice for any kind of library-ish code, if
there are no explicit check of interrupt status, it will be
checked a by next operation which might be interrupted. in this
particular case, I agree rethrowing it as an unchecked exception
might be a good alternative.
-- Igor
On Feb 11, 2020, at 10:03 PM, Chris Plummer
<chris.plum...@oracle.com <mailto:chris.plum...@oracle.com>>
wrote:
Hi Igor,
I guess I fail to see the benefit of this. Who is going to
check the interrupt status of this thread and do something
meaningful with it? It seems we would want to immediately
propagate the failure by throwing a RuntimeException. This will
work well when called from a test since this is a common way to
fail a test. The other use of this code is by
VMProps.vmHasSAandCanAttach(). It looks like if a
RuntimeException is thrown the right thing will happen when
SafeMap.put() catches the exception (it catches all Throwables).
Chris
On 2/11/20 7:12 PM, Igor Ignatev wrote:
rather like this :
} catch (InterruptedException e) {
Thread.currentThread().interrupt();
return false; // assume not signed
}
— Igor
On Feb 11, 2020, at 6:15 PM, Chris Plummer
<chris.plum...@oracle.com <mailto:chris.plum...@oracle.com>>
wrote:
Like this?
} catch (InterruptedException e) {
Thread.currentThread().interrupt();
throw new RuntimeException(e);
}
Chris
On 2/11/20 2:23 PM, Igor Ignatyev wrote:
no, I meant to call Thread.currentThread().interrupt(),
calling that will restore interrupted state of the thread,
so an user of Platform class will be able to response to it
appropriately, w/ your current code, the fact that the
thread was interrupted will be missed, and in most cases it
is not right thing to do.
-- Igor
On Feb 11, 2020, at 2:02 PM, Chris Plummer
<chris.plum...@oracle.com
<mailto:chris.plum...@oracle.com>> wrote:
Hi Igor,
I'm not sure what you mean by restore the interrupt state.
Do you mean loop back to the waitFor() call?
thanks,
Chris
On 2/11/20 1:55 PM, Igor Ignatyev wrote:
Hi Chris,
I don't insist on (3), so I'm fine if you don't want to
change that part. one thing I'd change though is to
restore thread interrupted state at L#266 of Platform.java
(no need to publish new webrev)
Thanks,
-- Igor
On Feb 11, 2020, at 1:49 PM, Chris Plummer
<chris.plum...@oracle.com
<mailto:chris.plum...@oracle.com>> wrote:
Hi Igor,
Here's an updated webrev:
http://cr.openjdk.java.net/~cjplummer/8238196/webrev.01/index.html
I rebased to JDK 15 and made all the changes you
suggested except for (3). I did not think it is necessary
since the code is only executed on OSX. However, if you
still feel allowing flexibility in the path separator is
important, I can add that change too.
thanks,
Chris
On 2/10/20 1:34 PM, Igor Ignatyev wrote:
Hi Chris,
in general it all looks good, I have a few comments
(most of them are editorial):
in Platform.java:
1. you have doubled spaced at line#238 (b/w boolean
and isSignedOSX)
2. as FileNotFoundException is IOException, there is no
need to declare the former in the signature of isSignedOSX
3. it's better to pass jdkPath, "bin" and "java" as
separate arguments to Path.get, so the code won't depend
on file separator
4. you are waiting for codesign to finish w/o reading
its cout / cerr, which might lead to a deadlock
(if codesign will exhaust IO buffer before exiting), so
you need to either create two separate threads to read
cout and cerr or redirect these streams them to files
and read these files afterwards or just ignore cout/cerr
by using Redirect.DISCARD. I'd personally recommend the
latter as the result of codesign can be reliably deduced
from its exitcode (0 - signed, 1 - verification failed,
2 - wrong arguments, 3 - not all requirements from R are
satisfied) and using cout/cerr is somewhat fragile as
there is no guarantee output format won't be changed.
the rest looks good to me.
-- Igor
On Feb 10, 2020, at 11:48 AM, Chris Plummer
<chris.plum...@oracle.com
<mailto:chris.plum...@oracle.com><mailto:chris.plum...@oracle.com>>
wrote:
Ping #2. It's not that hard of a review. Most of it is
the new Platform.isSignedOSX() method, which is well
commented and pretty straight froward.
thanks,
Chris
On 2/4/20 5:04 PM, Chris Plummer wrote:
Ping!
And I decided to push to 15 instead of 14. Will
backport to 14 eventually.
thanks,
Chris
On 1/30/20 10:20 PM, Chris Plummer wrote:
Yes, you are correct:
https://bugs.openjdk.java.net/browse/JDK-8238196
http://cr.openjdk.java.net/~cjplummer/8238196/webrev.00
thanks,
Chris
On 1/30/20 10:13 PM, Igor Ignatyev wrote:
Hi Chris,
http://cr.openjdk.java.net/~cjplummer/8236913/webrev.00Â
<http://cr.openjdk.java.net/~cjplummer/8236913/webrev.00%C3%82> seems
to be a webrev from another issue, should it have
beenÂhttp://cr.openjdk.java.net/~cjplummer/8238196/webrev.00/Â
<http://cr.openjdk.java.net/~cjplummer/8238196/webrev.00/%C3%82> ?
-- Igor
On Jan 30, 2020, at 10:10 PM, Chris Plummer
<chris.plum...@oracle.com
<mailto:chris.plum...@oracle.com><mailto:chris.plum...@oracle.com>>
wrote:
Hello,
Please review the following fix for some SA tests
that are failing on Mac OS X 10.14.5 and later:
https://bugs.openjdk.java.net/browse/JDK-8238196
http://cr.openjdk.java.net/~cjplummer/8236913/webrev.00
The issue is that SA can't attach to a signed
binary starting with 10.14.5. There is no
workaround for this, so these tests are being
disabled when it is detected that the binary is
signed and we are running on 10.14 or later (I
chose all 10.14 releases to simplify the check).
Some background may help explain the fix. In order
for SA to attach to a live process (not a core
file) on OSX, either the attaching process (ie. the
test) has to be run as root, or sudo needs to be
supported. However, the only tests that make the
sudo check are the 20 or so that use
ClhsdbLauncher. The rest all rely on "@requires
vm.hasSAandCanAttach" to filter out tests that use
SA attach. vm.hasSAandCanAttach only checks if the
test is being run as root. Thus all our
non-ClhsdbLauncher tests that SA attach to a live
process are currently not run unless they are run
as root. 8238268 [1] has been filed to address
this, making it so all the tests will attempt to
use sudo if not run as root.
Because of the difference in how ClhsdbLauncher
tests and "@requires vm.hasSAandCanAttach" tests
check to see if they are runnable, this fix needs
to address both types of checks. The common code
for both these cases is Platform.shouldSAAttach(),
which on OSX basically equates to check to see if
we are running as root. I changed it to also return
false if running on signed binary with 10.14 or
later. However, this confused the ClhsdbLauncher
use of Platform.shouldSAAttach() somewhat, since it
assumed a false result only happens because you are
not running as root (in which case it would then
check if sudo will work). So ClhsdbLauncher now has
double check that the false result was not because
of running a signed binary. If it is signed, it
won't do the sudo check. This will get cleaned up
with 8238268 [1], which will move the sudo check
into Platform.shouldSAAttach().
thanks,
Chris
[1] https://bugs.openjdk.java.net/browse/JDK-8238268