RFR 8208715: Conversion of milliseconds to nanoseconds in UNIXProcess contains bug

[Roger Riggs]

Hi Martin,

ok, as a reliable pattern I see that to avoid knowing details of the 
implementation.
But the self interrupt seems redundant if the other is needed.

Updated:
   http://cr.openjdk.java.net/~rriggs/webrev-timeout-8208715/

Thanks, Roger


On 8/14/2018 6:14 PM, Martin Buchholz wrote:
> Looks good to me ... but ...
>
> my intent was that the self-interrupt was added in __addition__ to 
> interrupt by other thread, because there is often different code to 
> handle pre-existing interrupt.  As with 
> BlockingQueueTest.testTimedPollWithOffer.
>
>                 public void run() {
>                     Thread.currentThread().interrupt();
>                     try {
>                         boolean result = p.waitFor(Long.MAX_VALUE, 
> TimeUnit.MILLISECONDS);
>                         fail("Should have thrown InterruptedException");
>                     } catch (InterruptedException success) {
>                     } catch (Throwable t) { unexpected(t); }
>
>                     try {
>                         aboutToWaitFor.countDown();
>                         boolean result = p.waitFor(Long.MAX_VALUE, 
> TimeUnit.MILLISECONDS);
>                         fail("Should have thrown InterruptedException");
>                     } catch (InterruptedException success) {
>                     } catch (Throwable t) { unexpected(t); }
>
>
>
>
> On Tue, Aug 14, 2018 at 12:59 PM, Roger Riggs <Roger.Riggs at oracle.com 
> <mailto:Roger.Riggs at oracle.com>> wrote:
>
>     Hi Martin,
>
>     Updated with suggestions:
>     http://cr.openjdk.java.net/~rriggs/webrev-timeout-8208715/index.html
>     <http://cr.openjdk.java.net/%7Erriggs/webrev-timeout-8208715/index.html>
>
>     On 8/14/2018 1:22 PM, Martin Buchholz wrote:
>>     Thanks, Roger.  I approve this version, although as always I have
>>     comments.
>>
>>      520     private static native void waitForTimeoutInterruptibly(
>>      521         long handle, long timeout);
>>
>>     The units being used should be obvious from the signature -
>>     rename timeout to timeoutMillis.
>     Done
>>     But even better is to push the support for nanos into the utility
>>     method, so change this native method to accept a timeoutNanos.
>     A bit far from the original issue and it might take a bit more time.
>>
>>     2465             Thread.sleep(1000);
>>
>>     I hate sleeps, and I would just delete this one - I don't think
>>     the test relies on it (and if it did, one second is not long
>>     enough!).
>     Done.  (And in the test case used for the copy/paste of the new test).
>>
>>     2454                     try {
>>     2455  aboutToWaitFor.countDown();
>>     2456                         boolean result =
>>     p.waitFor(Long.MAX_VALUE, TimeUnit.MILLISECONDS);
>>     2457                         fail("waitFor() wasn't interrupted,
>>     its return value was: " + result);
>>     2458                     } catch (InterruptedException success) {
>>     2459                     } catch (Throwable t) { unexpected(t); }
>>
>>     It's easy to add a self-interrupt variant inside the run method
>>
>>     Thread.currentThread().interrupt(); ...
>     ok, and will run all the tests again.
>
>     Thanks, Roger
>
>
>>
>>     (TODO: Basic.java is in need of a re-write - mea culpa ...)
>>
>>
>>
>>
>>
>>     On Tue, Aug 14, 2018 at 9:48 AM, Roger Riggs
>>     <Roger.Riggs at oracle.com <mailto:Roger.Riggs at oracle.com>> wrote:
>>
>>         Hi Martin,
>>
>>         I updated the webrev with the suggestions.
>>
>>         On 8/14/2018 10:47 AM, Martin Buchholz wrote:
>>>         hi Roger,
>>>
>>>         509 if (deadline <= 0) {
>>>         510 deadline = Long.MAX_VALUE;
>>>         511 }
>>>
>>>         This must be wrong.  Nanotime wraparound is normal in this
>>>         sort of code.
>>         ok, this reader didn't make that assumption
>>>
>>>         ---
>>>
>>>         We ought to be able to delegate the fiddling with nanos to
>>>         TimeUnit.timedWait.
>>>
>>>         Does it work to simply call
>>>         NANOSECONDS.timedWait(remainingNanos) ?
>>>         If not, is there a bug in TimeUnit.timedWait?
>>         That works except on Windows, that does not use wait().
>>
>>>
>>>         It's good to add a test for this.  I've tried hard in
>>>         similar tests to avoid sleep and to add variants where the
>>>         target thread is interrupted before and after starting to
>>>         wait.  Testing pre-interrupt is easy - the thread can
>>>         interrupt itself. BlockingQueueTest.testTimedPollWithOffer
>>>         is an example.
>>         I added a test, using the same logic as the existing tests
>>         for the Long.MAX_VALUE case
>>
>>         Thanks, Roger
>>
>>>
>>>
>>>
>>>
>>>         On Tue, Aug 14, 2018 at 7:00 AM, Roger Riggs
>>>         <Roger.Riggs at oracle.com <mailto:Roger.Riggs at oracle.com>> wrote:
>>>
>>>             Please review a fix for
>>>             Process.waitFor(Long.MAX_VALUE,MILLIS).
>>>             Catch wrap around in very large wait times and saturate
>>>             at Long.MAX_VALUE.
>>>
>>>             Webrev:
>>>             http://cr.openjdk.java.net/~rriggs/webrev-timeout-8208715/
>>>             <http://cr.openjdk.java.net/%7Erriggs/webrev-timeout-8208715/>
>>>
>>>             Issue:
>>>             https://bugs.openjdk.java.net/browse/JDK-8208715
>>>             <https://bugs.openjdk.java.net/browse/JDK-8208715>
>>>
>>>             Thanks, Roger
>>>
>>>
>>
>>
>
>