RFR: 8029525 - java/lang/ProcessBuilder/Basic.java fails intermittently

[Chris Hegarty]

> On 5 Dec 2013, at 19:18, Martin Buchholz <martinrb at 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 <martinrb at 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.mckenna at 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
>>