Hi Chris,
I think you are overthinking this. :)
What you have observed is that the code that actually uses this method
does not utilise interrupts, or expect them, so if you artifically
inject one in this library method then you see things failing in
unexpected ways. That also means that if the thread was interrupted by
some other piece of logic then it would also fail in unexpected ways.
That doesn't negate your choice to re-assert the interrupt state.
From a library writing perspective if you have a method that performs a
blocking call that can throw InterruptedException then you generally
have three choices:
1. Throw InterruptedException yourself and pass the buck to your callers.
2. Convert the InterruptedException to a more general failure exception
- typically an unchecked RuntimeException - for which interruption is
but one possible cause; or
3. Catch the InterruptedException and allow the method to complete
normally (i.e. not by throwing an exception) but re-assert the interrupt
state so that a caller checking for interruption will still see that it
occurred.
What you have below is a mix of #2 and #3 - you convert to a generic
exception but also re-assert the interrupt state. That's a little unusual.
David
On 12/02/2020 6:16 pm, Chris Plummer wrote:
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
