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

[Jaroslav Bachorik]

Hi David,

On 9.3.2015 12:15, David Holmes wrote:
> On 9/03/2015 5:37 PM, Jaroslav Bachorik wrote:
>> 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.
>
> Yes.
>
>>>
>>> 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.
>
> It is the actual implementation I'm concerned about - to ensure the
> notions of "alive" actually match.

Invoking ThreadMXBean#getAllThreadIds() will eventually mean creating a 
ThreadsListEnumerator (in threadService.cpp#941) and this enumerator 
will skip any threads where `Thread.isAlive() == false`. The 
Thread.join() method is using Thread.isAlive() to assert the thread 
state and exits only when `Thread.isAlive() == false`. Therefore, a 
thread T can't be in the list of live threads generated after T.join() 
has returned.

-JB-

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