> On 5 Dec 2013, at 19:18, Martin Buchholz <marti...@google.com> wrote:
>
> You guys are way too trigger-happy.
I guess the reason for this is that there was a deadline today to get changes
into tl before the b120 snapshot. This is one of the final chances to get
non-showstopper changes into JDK 8.
Your patch will probably have to wait until JDK 9 opens :-(
-Chris.
>
> Rob, I propose you add this patch, that seems cleaner to me, and also
> covers the previously untested case of interrupt before waitFor (at the
> cost of more code duplication).
>
> diff --git a/test/java/lang/ProcessBuilder/Basic.java
> b/test/java/lang/ProcessBuilder/Basic.java
> --- a/test/java/lang/ProcessBuilder/Basic.java
> +++ b/test/java/lang/ProcessBuilder/Basic.java
> @@ -58,6 +58,15 @@
> /* used for Mac OS X only */
> static final String cfUserTextEncoding =
> System.getenv("__CF_USER_TEXT_ENCODING");
>
> + /**
> + * Returns the number of milliseconds since time given by
> + * startNanoTime, which must have been previously returned from a
> + * call to {@link System.nanoTime()}.
> + */
> + private static long millisElapsedSince(long startNanoTime) {
> + return TimeUnit.NANOSECONDS.toMillis(System.nanoTime() -
> startNanoTime);
> + }
> +
> private static String commandOutput(Reader r) throws Throwable {
> StringBuilder sb = new StringBuilder();
> int c;
> @@ -2232,40 +2241,66 @@
>
> //----------------------------------------------------------------
> // Check that Process.waitFor(timeout, TimeUnit.MILLISECONDS)
> - // interrupt works as expected.
> + // interrupt works as expected, if interrupted while waiting.
> //----------------------------------------------------------------
> try {
> List<String> childArgs = new ArrayList<String>(javaChildArgs);
> childArgs.add("sleep");
> final Process p = new ProcessBuilder(childArgs).start();
> final long start = System.nanoTime();
> - final CountDownLatch ready = new CountDownLatch(1);
> - final CountDownLatch done = new CountDownLatch(1);
> + final CountDownLatch aboutToWaitFor = new CountDownLatch(1);
>
> final Thread thread = new Thread() {
> public void run() {
> try {
> - final boolean result;
> - try {
> - ready.countDown();
> - result = p.waitFor(30000,
> TimeUnit.MILLISECONDS);
> - } catch (InterruptedException e) {
> - return;
> - }
> + aboutToWaitFor.countDown();
> + boolean result = p.waitFor(30L * 1000L,
> TimeUnit.MILLISECONDS);
> fail("waitFor() wasn't interrupted, its return
> value was: " + result);
> - } catch (Throwable t) {
> - unexpected(t);
> - } finally {
> - done.countDown();
> - }
> + } catch (InterruptedException success) {
> + } catch (Throwable t) { unexpected(t); }
> }
> };
>
> thread.start();
> - ready.await();
> + aboutToWaitFor.await();
> Thread.sleep(1000);
> thread.interrupt();
> - done.await();
> + thread.join(10L * 1000L);
> + check(millisElapsedSince(start) < 10L * 1000L);
> + check(!thread.isAlive());
> + p.destroy();
> + } catch (Throwable t) { unexpected(t); }
> +
> + //----------------------------------------------------------------
> + // Check that Process.waitFor(timeout, TimeUnit.MILLISECONDS)
> + // interrupt works as expected, if interrupted before waiting.
> + //----------------------------------------------------------------
> + try {
> + List<String> childArgs = new ArrayList<String>(javaChildArgs);
> + childArgs.add("sleep");
> + final Process p = new ProcessBuilder(childArgs).start();
> + final long start = System.nanoTime();
> + final CountDownLatch threadStarted = new CountDownLatch(1);
> +
> + final Thread thread = new Thread() {
> + public void run() {
> + try {
> + threadStarted.countDown();
> + do { Thread.yield(); }
> + while (!Thread.currentThread().isInterrupted());
> + boolean result = p.waitFor(30L * 1000L,
> TimeUnit.MILLISECONDS);
> + fail("waitFor() wasn't interrupted, its return
> value was: " + result);
> + } catch (InterruptedException success) {
> + } catch (Throwable t) { unexpected(t); }
> + }
> + };
> +
> + thread.start();
> + threadStarted.await();
> + thread.interrupt();
> + thread.join(10L * 1000L);
> + check(millisElapsedSince(start) < 10L * 1000L);
> + check(!thread.isAlive());
> p.destroy();
> } catch (Throwable t) { unexpected(t); }
>
>
>
>
>
>> On Thu, Dec 5, 2013 at 9:44 AM, Martin Buchholz <marti...@google.com> wrote:
>>
>> sorry for leaving so many little problems in this test.
>>
>> (In my defense, ProcessBuilder is hard to test in a non-flaky way.)
>>
>> I think we can come up with a cleaner fix. I'll post a patch later.
>>
>>
>> On Thu, Dec 5, 2013 at 6:19 AM, Rob McKenna <rob.mcke...@oracle.com>wrote:
>>
>>> This failure cropped up again and Roger Riggs spotted that I was looking
>>> at it from completely the wrong direction. He contributed the following fix:
>>>
>>> http://cr.openjdk.java.net/~robm/8029525/webrev.01/
>>>
>>> This is to avoid a race between:
>>>
>>> thread.interrupt();
>>> p.destroy();
>>>
>>> Hoping to get this reviewed and pushed as soon as possible!
>>>
>>> Thanks,
>>>
>>> -Rob
>>
