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

[Martin Buchholz]

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.  But even better is to push the support for nanos into
the utility method, so change this native method to accept a timeoutNanos.

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!).

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(); ...

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