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

Roger Riggs Roger.Riggs at Oracle.com
Wed Aug 15 14:06:18 UTC 2018

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_ _<mailto:Roger.Riggs at oracle.com>> wrote: Hi Martin, Updated with suggestions: http://cr.openjdk.java.net/~rriggs/webrev-timeout-8208715/index.html <http://cr.openjdk.java.net/%7Erriggs/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 <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.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 <mailto: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/ <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