|
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
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
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>>
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Â seems
to be a webrev from another issue,
should it have been http://cr.openjdk.java.net/~cjplummer/8238196/webrev.00/ ?
-- Igor
On
Jan 30, 2020, at 10:10 PM, Chris
Plummer <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
|
