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

[Martin Buchholz]

Your change is still approved, but ...

All tests are a little white-box in the sense that they are guided by where
bugs are likely to be found, which comes from experience with
implementations.  There is a collection of timed waiting APIs in the JDK
that throw InterruptedException.  It would be great if the entire test
matrix was covered: wait time 0, small positive wait times out, maximally
negative wait time, maximally positive wait time, interrupted before
waiting, interrupted while blocked in wait (Thread state not RUNNING), and
probably more.  We don't achieve full coverage anywhere, but it can be a
goal.

On Wed, Aug 15, 2018 at 7:06 AM, Roger Riggs <Roger.Riggs at oracle.com> wrote:

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