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

[Martin Buchholz]

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

> Hi Martin,
>
> Updated with suggestions:
>  http://cr.openjdk.java.net/~rriggs/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>
> 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>
>> 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/
>>>
>>> Issue:
>>> https://bugs.openjdk.java.net/browse/JDK-8208715
>>>
>>> Thanks, Roger
>>>
>>>
>>
>>
>
>