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

[Martin Buchholz]

I understand y'all luhv stability, but ... it's ONLY A TEST!

Also, there's Martin's 4th Law of Software Development:
Developers must always have a place to submit-and-forget good changes.


On Thu, Dec 5, 2013 at 11:42 AM, Chris Hegarty <chris.hegarty at oracle.com>wrote:

>
> > 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
> >>
>