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