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

[Daniel Fuchs]

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-