Re: RFR: 8029525 - java/lang/ProcessBuilder/Basic.java fails intermittently
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 wrote:
On 5 Dec 2013, at 19:18, Martin Buchholz 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 childArgs = new
ArrayList(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(3,
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 childArgs = new
ArrayList(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
Re: RFR: 8029525 - java/lang/ProcessBuilder/Basic.java fails intermittently
On 05/12/2013 19:42, Chris Hegarty wrote: On 5 Dec 2013, at 19:18, Martin Buchholz 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. Just to clarify. Only show stopper bugs have been allowed since Nov 21 when the JDK 8 project went into rampdown phase 2 [1]. There was an exception proposed for documentation and test bugs [2] and several people have been making use of that exception to fix test issues. This is very welcome because tests just don't always get the TLC that they deserve. The exception for documentation and test fixes goes to Dec 12 but the rub is that they have to be in the master (jdk8/jdk8) by that date. Changes pushed to jdk8/tl are usually integrated into master on a Tuesday [3] and even less obvious is that this requires taking a snapshot a few days in advance of that to allow for build + integration testing (an activity that happens to be getting some air time on jdk9-dev list at the moment). I really wish this was more obvious and the dates/cut-offs were clearly announced - this is something that we have to improve on and fix in JDK 9. In any case, if Martin's test update has missed the last bus to make the Dec 12 deadline then it shouldn't be long before there is a jdk9 forest open for business. -Alan. [1] http://openjdk.java.net/projects/jdk8/milestones [2] http://mail.openjdk.java.net/pipermail/jdk8-dev/2013-October/003443.html [3] http://openjdk.java.net/projects/jdk8/builds
Re: RFR: 8029525 - java/lang/ProcessBuilder/Basic.java fails intermittently
> On 5 Dec 2013, at 19:18, Martin Buchholz 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 childArgs = new ArrayList(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(3,
> 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 childArgs = new ArrayList(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); }
> +}
> +
Re: RFR: 8029525 - java/lang/ProcessBuilder/Basic.java fails intermittently
On 05/12/2013 14:19, Rob McKenna 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! The change looks okay, I guess I would rename "latch" to something like "ready" (as ready+done might be nicer than latch+done). If you have more time then you might look at using a phaser. It might also be a bit simpler if the waitFor is done in the main thread and have the background thread do the interrupt. In any case, it's good to make this part of the test more robust as it has been troublesome. -Alan.
Re: RFR: 8029525 - java/lang/ProcessBuilder/Basic.java fails intermittently
On 5 Dec 2013, at 14:40, roger riggs wrote: > Hi Rob, > > Looks fine. (Not a Reviewer). +1. Looks ok to me too. -Chris. > > Thanks for doing the legwork, Roger > > On 12/5/2013 9:19 AM, Rob McKenna 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 >> >
Re: RFR: 8029525 - java/lang/ProcessBuilder/Basic.java fails intermittently
Hi Rob, Looks fine. (Not a Reviewer). Thanks for doing the legwork, Roger On 12/5/2013 9:19 AM, Rob McKenna 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
RFR: 8029525 - java/lang/ProcessBuilder/Basic.java fails intermittently
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
