RFR: 6815130 intermittent ThreadMXBean/Locks.java failure

[Jaroslav Bachorik]

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