RFR 8208715: Conversion of milliseconds to nanoseconds in UNIXProcess contains bug (original) (raw)

Martin Buchholz martinrb at google.com
Wed Aug 15 14:22:40 UTC 2018

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.MAXVALUE, TimeUnit.MILLISECONDS); fail("Should have thrown InterruptedException"); } catch (InterruptedException success) { } catch (Throwable t) { unexpected(t); } try { aboutToWaitFor.countDown(); boolean result = p.waitFor(Long.MAXVALUE, 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.MAXVALUE, 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.MAXVALUE; 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.MAXVALUE 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.MAXVALUE,MILLIS). Catch wrap around in very large wait times and saturate at Long.MAXVALUE. Webrev: http://cr.openjdk.java.net/~rriggs/webrev-timeout-8208715/ Issue: https://bugs.openjdk.java.net/browse/JDK-8208715 Thanks, Roger

More information about the core-libs-dev mailing list