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> 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 >>>>>>>>> <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> >>>>>>> >>>>>> >>>>> >>> >> >
