RFR 8208715: Conversion of milliseconds to nanoseconds in UNIXProcess contains bug
Roger Riggs
Roger.Riggs at Oracle.com
Tue Aug 14 19:59:48 UTC 2018
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
> <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
>>
>>
>
>
More information about the core-libs-dev
mailing list