RFR 6712222: 6712222: Race condition in java/lang/management/ThreadMXBean/AllThreadIds.java

[Jaroslav Bachorik]

On 8.3.2015 14:32, David Holmes wrote:
> Hi Jaroslav,
>
> A couple of quick comments ...
>
> On 7/03/2015 2:08 AM, Jaroslav Bachorik wrote:
>> Please, review the following test change
>>
>> Issue : https://bugs.openjdk.java.net/browse/JDK-6712222
>> Webrev: http://cr.openjdk.java.net/~jbachorik/6712222/webrev.00
>>
>> As the issue subject states this test is susceptible to race condition.
>> Firstly, it uses an arbitrary time period to wait after a thread has
>> signalled its exit vie the associated barrier to 'make sure' the thread
>> is actually terminated when the check for the number of live threads is
>> performed. This is quite race prone and should be replaces by
>> Thread.join() performed on the specified threads before moving forward
>> and checking the reported thread counts.
>
> So now you do the join() why do you still also have an acquire()
> preceding the join(s) ?

Touché - I guess I just didn't want to mess with the test too much. I 
suppose joining the threads that are expected to die should suffice.

>
> Also what exactly do those counters check? Even after join() can return
> the thread may still be in the Threads_list.

The test is supposed to test whether ThreadMXBean reports correct 
numbers for the total number of started/finished threads and the number 
of threads being currently alive.

Thread#join() should return only when the thread is not alive any more. 
The ThreadMXBean#getAllThreadIds() should return the IDs of only the 
threads being alive at the time the method was invoked. I didn't dig 
into the implementation - but according to the information available 
through javadocs this should work.

-JB-

>
> David
>
>> Secondly, it is using a volatile array of boolean flags which are used
>> for communication between the main thread and the test threads (setting
>> an item of this array true will indicate the appropriate test thread to
>> exit). Declaring the array as volatile does nothing for the thread
>> safety and reads and writes to this array must be properly synchronized.
>>
>> I also took the liberty of replacing the arbitrary Barrier
>> synchronization class with the standard Semaphore.
>>
>>
>> Thanks,
>>
>> -JB-