jmx-dev RFR: 6815130 intermittent ThreadMXBean/Locks.java failure

[David Holmes]

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.

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-
>>
>