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

[Martin Buchholz]

You guys are way too trigger-happy.

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