Re: Race condition in TimerTask KillThread test
On 10/11/2011 9:50 PM, Gary Adams wrote: On 11/10/11 2:22 AM, David Holmes wrote: Aside: it would be good if whomever is going to do the commits for you could assist with publishing webrevs for these changes. There was a bulk request to add Vivian's team to the openjdk authors list that may have taken a longer route by way of Bob to Mark. I'll gladly post webrevs to java.net as soon as the administration tasks have been completed. I was mistakenly thinking you had to be a Committer to get the openJDK user name etc, but thankfully that is not the case. David
Re: Race condition in TimerTask KillThread test
On 11/10/11 2:22 AM, David Holmes wrote: Aside: it would be good if whomever is going to do the commits for you could assist with publishing webrevs for these changes. There was a bulk request to add Vivian's team to the openjdk authors list that may have taken a longer route by way of Bob to Mark. I'll gladly post webrevs to java.net as soon as the administration tasks have been completed.
Re: Race condition in TimerTask KillThread test
Hi Gary,
On 10/11/2011 5:36 AM, Gary Adams wrote:
Here's a revised diff for the KillThread timing problem :
- added current CR# to @bug tag
I don't think that is correct. AFAIK the @bug indicates what bug this
test is testing the fix for, not which bugs modified the test. (Ditto
for your other test fixes).
Alan: can you confirm? Thanks.
public class KillThread {
+ static Thread tdThread;
Must be volatile as its value is being communicated from the timer
thread to the main thread.
public static void main (String[] args) throws Exception {
+ tdThread = null;
Not necessary - static field is null by default.
Timer t = new Timer();
// Start a mean event that kills the timer thread
t.schedule(new TimerTask() {
public void run() {
+ tdThread = Thread.currentThread();
throw new ThreadDeath();
}
}, 0);
// Wait for mean event to do the deed and thread to die.
try {
+ do {
Thread.sleep(100);
+ } while(tdThread == null);
} catch(InterruptedException e) {
}
Minor nit: You didn't introduce this, but catching IE is unnecessary as
main can let it propagate. With the updated code in the unlikely event
it occurred you might then go and invoke join() on a null reference.
Aside: it would be good if whomever is going to do the commits for you
could assist with publishing webrevs for these changes.
Thanks,
David
-
+ tdThread.join();
// Try to start another event
try {
On 11/ 6/11 08:46 PM, David Holmes wrote:
On 5/11/2011 8:37 AM, Alan Bateman wrote:
On 04/11/2011 15:52, Gary Adams wrote:
:
I'll attempt a simple fix for the KillThread case by replacing :
Thread.sleep(100);
with a simple loop
do {
Thread.sleep(100);
} while (waiting);
where the TimerTask runnable will clear the flag
with "waiting = false " once it has been launched.
I don't think this will guarantee that the timer thread will have
terminated before the main thread schedules the second event. I think
this test would be robust if you capture a reference to the timer thread
in the first task and have the main thread wait or poll until it has a
reference to the thread. Once it has a reference to the thread then it
can wait for it to terminate before attempting to schedule the second
task.
Agreed. As soon as waiting is set we can switch back to the main
thread and proceed to the next phase of the test - but the timer
thread is still alive at that point. If the issue is checking for
thread termination then we must track that directly.
David
-
-Alan.
Re: Race condition in TimerTask KillThread test
Here's a revised diff for the KillThread timing problem :
- added current CR# to @bug tag
- capture the thread from the timer task
- wait for the timertask thread to be visible to the main thread
then join the thread before fall through to attempt the second
timertask schedule call.
diff --git a/test/java/util/Timer/KillThread.java
b/test/java/util/Timer/KillThread.java
--- a/test/java/util/Timer/KillThread.java
+++ b/test/java/util/Timer/KillThread.java
@@ -23,7 +23,7 @@
/*
* @test
- * @bug 4288198
+ * @bug 4288198 6818464
* @summary Killing a Timer thread causes the Timer to fail silently on
* subsequent use.
*/
@@ -31,21 +31,27 @@
import java.util.*;
public class KillThread {
+static Thread tdThread;
public static void main (String[] args) throws Exception {
+tdThread = null;
Timer t = new Timer();
// Start a mean event that kills the timer thread
t.schedule(new TimerTask() {
public void run() {
+tdThread = Thread.currentThread();
throw new ThreadDeath();
}
}, 0);
// Wait for mean event to do the deed and thread to die.
try {
+do {
Thread.sleep(100);
+} while(tdThread == null);
} catch(InterruptedException e) {
}
+tdThread.join();
// Try to start another event
try {
On 11/ 6/11 08:46 PM, David Holmes wrote:
On 5/11/2011 8:37 AM, Alan Bateman wrote:
On 04/11/2011 15:52, Gary Adams wrote:
:
I'll attempt a simple fix for the KillThread case by replacing :
Thread.sleep(100);
with a simple loop
do {
Thread.sleep(100);
} while (waiting);
where the TimerTask runnable will clear the flag
with "waiting = false " once it has been launched.
I don't think this will guarantee that the timer thread will have
terminated before the main thread schedules the second event. I think
this test would be robust if you capture a reference to the timer thread
in the first task and have the main thread wait or poll until it has a
reference to the thread. Once it has a reference to the thread then it
can wait for it to terminate before attempting to schedule the second task.
Agreed. As soon as waiting is set we can switch back to the main thread and
proceed to the next phase of the test - but the timer thread is still alive at
that point. If the issue is checking for thread termination then we must track
that directly.
David
-
-Alan.
Re: Race condition in TimerTask KillThread test
On 5/11/2011 1:52 AM, Gary Adams wrote:
I'm taking a look at some older timing based bugs that may cause problems
on slower machines
6818464: TEST_BUG: Timeout values in several java/util tests are
insufficient
I'd like to split this bug into two, based on the example problems that are
mentioned in the bug report.
The first example in
java/util/Timer/KillThread.java
In the second example the test instructions present a timeout
to be enforced by the outer test harness. e.g.
@run main/timeout=20 StoreDeadLock
This type of test limit is easily addressed on slower machines
using the test harness timefactor to scale the acceptable
test run time.
Not sure that is the case in general. Use of timefactor was suggested
when running in Xcomp mode because the top-level test configuration
knows it is running the tests in Xcomp mode. But in general the test
harness does not know when any given test is running on a slower
machine, or a not-so-slow but heavily loaded machine.
David
-
I'll attempt a simple fix for the KillThread case by replacing :
Thread.sleep(100);
with a simple loop
do {
Thread.sleep(100);
} while (waiting);
where the TimerTask runnable will clear the flag
with "waiting = false " once it has been launched.
Re: Race condition in TimerTask KillThread test
On 5/11/2011 8:37 AM, Alan Bateman wrote:
On 04/11/2011 15:52, Gary Adams wrote:
:
I'll attempt a simple fix for the KillThread case by replacing :
Thread.sleep(100);
with a simple loop
do {
Thread.sleep(100);
} while (waiting);
where the TimerTask runnable will clear the flag
with "waiting = false " once it has been launched.
I don't think this will guarantee that the timer thread will have
terminated before the main thread schedules the second event. I think
this test would be robust if you capture a reference to the timer thread
in the first task and have the main thread wait or poll until it has a
reference to the thread. Once it has a reference to the thread then it
can wait for it to terminate before attempting to schedule the second task.
Agreed. As soon as waiting is set we can switch back to the main thread
and proceed to the next phase of the test - but the timer thread is
still alive at that point. If the issue is checking for thread
termination then we must track that directly.
David
-
-Alan.
Re: Race condition in TimerTask KillThread test
On 04/11/2011 15:52, Gary Adams wrote:
:
I'll attempt a simple fix for the KillThread case by replacing :
Thread.sleep(100);
with a simple loop
do {
Thread.sleep(100);
} while (waiting);
where the TimerTask runnable will clear the flag
with "waiting = false " once it has been launched.
I don't think this will guarantee that the timer thread will have
terminated before the main thread schedules the second event. I think
this test would be robust if you capture a reference to the timer thread
in the first task and have the main thread wait or poll until it has a
reference to the thread. Once it has a reference to the thread then it
can wait for it to terminate before attempting to schedule the second task.
-Alan.
Race condition in TimerTask KillThread test
I'm taking a look at some older timing based bugs that may cause problems
on slower machines
6818464: TEST_BUG: Timeout values in several java/util tests are insufficient
I'd like to split this bug into two, based on the example problems that are
mentioned in the bug report.
The first example in
java/util/Timer/KillThread.java
is a legitimate race condition. The code only will work
correctly if the processing for the TimerTask schedules
and fires the runnable within the hard coded
100 milliseconds. This can be fixed with a local variable
to synchronize when the the second operation can be
attempted. (Hard coded sleep times are rarely correct
when dealing with slower devices.)
In the second example the test instructions present a timeout
to be enforced by the outer test harness. e.g.
@run main/timeout=20 StoreDeadLock
This type of test limit is easily addressed on slower machines
using the test harness timefactor to scale the acceptable
test run time.
I'll attempt a simple fix for the KillThread case by replacing :
Thread.sleep(100);
with a simple loop
do {
Thread.sleep(100);
} while (waiting);
where the TimerTask runnable will clear the flag
with "waiting = false " once it has been launched.
