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
