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

[David Holmes]

Seems good to go to me.

David

On 5/09/2013 10:52 PM, Jaroslav Bachorik wrote:
> On 09/04/2013 10:33 AM, Jaroslav Bachorik wrote:
>> 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.
>
> Task for removal of
> java/lang/management/ThreadMXBean/ThreadExecutionSynchronizer filed -
> JDK-8024326
>
> So, is this change good to go?
>
> Thanks,
>
> -JB-
>
>>
>> -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-
>>>>>
>>>>
>>
>