On 09/04/2013 10:29 AM, David Holmes wrote: > On 4/09/2013 4:56 PM, 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. > > Yes that seems valid. > >> 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. > > Yes that seems good too. > >> The very simplified patch is at >> http://cr.openjdk.java.net/~jbachorik/6815130/webrev.03 > > One thing you "fixed" in the original version but is no longer "fixed" > is findOwnerInfo where you made the break exit both loops. I'm unclear > if that is a correctness issue or just an optimization? I suspect an > optimization and that once you have found what you need, > lock.equals(blockedLock) will not be true for any other ThreadInfo objects.
Exactly. It was an optimization and I excluded the change to keep the fix very simple - given that the whole test would very likely be rewritten for JDK9. -JB- > > Thanks, > David > >> 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- >>> >>
