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

[roger riggs]

Created an issue and attached the patch so it doesn't get lost.

https://bugs.openjdk.java.net/browse/JDK-8029629

Roger

On 12/5/2013 3:12 PM, Martin Buchholz wrote:
> 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