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> 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 >>> <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 >>>>> <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 >>>>>>> <https://bugs.openjdk.java.net/browse/JDK-8238196> >>>>>>> http://cr.openjdk.java.net/~cjplummer/8238196/webrev.00 >>>>>>> <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 >>>>>>>>> <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 >>>>>>>>> <https://bugs.openjdk.java.net/browse/JDK-8238196> >>>>>>>>> http://cr.openjdk.java.net/~cjplummer/8236913/webrev.00 >>>>>>>>> <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 >>>>>>>>> <https://bugs.openjdk.java.net/browse/JDK-8238268> >
