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

[David Holmes]

Hi Jaroslav,

On 3/09/2013 11:02 PM, Jaroslav Bachorik wrote:
> On 09/03/2013 02:10 PM, Daniel Fuchs wrote:
>> Hi Jaroslav,
>>
>> Have you considered replacing the ThreadExecutionSynchronizer with
>> a plain (and more reliable) CyclicBarrier or Phaser object?
>>
>> <http://docs.oracle.com/javase/7/docs/api/java/util/concurrent/CyclicBarrier.html>
>>
>> <http://docs.oracle.com/javase/7/docs/api/java/util/concurrent/Phaser.html>
>
> Yes, I've played with the idea of using more advanced concurrency
> constructs. But I couldn't fit the CyclicBarrier to the usecase and when
> I used Phaser the solution complexity was considerably higher. It might
> be due to my lack of experience with the Phaser, specifically, but the
> plain old 'synchronized' yielded the best results for me :(

Unfortunately your use of wait() does not account for spurious wakeups - 
which the existing Semaphore masks from you.

A CyclicBarrier would seem to be a better fit, but the logic in this 
test is convoluted enough that it is hard to tell exactly what the 
synchronization protocol is.

Also your gratuitous style changes made it very hard to see actual 
functional changes in the test. There's really no need to change for 
loops into for-each loops. ;-)

David
----

> -JB-
>
>>
>> best regards
>>
>> -- daniel
>>
>> On 9/3/13 1:15 PM, 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-
>>>
>>
>