Hi Jaroslav,
That looks like a reasonable small change if you are anxious to get that
fix out of your plate fast. I'll trust that your analysis is right and that
declaring waiting volatile is enough to fix the issue. It was certainly an
error to access 'waiting' from multiple threads without any kind of
synchronization.
I'll be happy to see ThreadExecutionSynchronizer go away in JDK 9 :-)
best regards,
-- daniel
On 9/4/13 8:56 AM, Jaroslav Bachorik wrote:
On 09/04/2013 04:24 AM, Mandy Chung wrote:
Hi Jaroslav,
Like Daniel and David said, CyclicBarrier and other j.u.concurrent
utility seem a good replacement with the ThreadExecutionSynchronizer
class. ThreadMXBean/Locks.java was written prior to j.u.concurrent
added to the platform (both java.util.concurrent and
java.lang.management were added in JDK 5). Later
ThreadExecutionSynchronizer was added to fix some intermittent issue.
I think it's worth the investigation to replace the existing logic with
j.u.concurrent and get rid of ThreadExecutionSynchronizer (which is used
by a few other tests).
Ok, let's go back to the basics :)
The reason for the test to fail intermittently are stale reads from the
"waiting" variable. In order to minimize the changes it seems sufficient
to make the "waiting" variable volatile to prevent the stale reads. The
code modifying the "waiting" variable is already guarded by the
semaphore so we are good there.
The changes in Locks.java are about more consistent approach to waiting
for a thread to enter a desired state. I took Erik's recommendation to
just wait indefinitely for the desired thread state and let the harness
deal with timeouts.
The very simplified patch is at
http://cr.openjdk.java.net/~jbachorik/6815130/webrev.03
I will file a task for JDK9 to remove ThreadExecutionSynchronizer and
simplify java.lang.management tests using it.
-JB-
Mandy
On 9/3/2013 4:15 AM, Jaroslav Bachorik wrote:
Please, review the following patch of the intermittently failing test:
http://cr.openjdk.java.net/~jbachorik/6815130/webrev.01
Issue: https://bugs.openjdk.java.net/browse/JDK-6815130
Sometimes the ThreadExecutionSynchronizer class failes to achieve the
desired synchronization (due to possible data races when evaluating and
setting the "waiting" variable) leading to test failures.
The patch fixes the possibility of data race. Also the Locks class is
tidied up a bit.
Thanks,
-JB-
