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.ri...@oracle.com
<mailto:roger.ri...@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.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.ri...@oracle.com <mailto:roger.ri...@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.ri...@oracle.com <mailto:roger.ri...@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
