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

[David Holmes]

On 9/03/2015 11:26 PM, Jaroslav Bachorik wrote:
> On 9.3.2015 13:41, Jaroslav Bachorik wrote:
>> 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.
>
> Here is the simplified webrev -
> http://cr.openjdk.java.net/~jbachorik/6712222/webrev.01
>
> Now, when it is sufficient to synchronize only on the threads startup I
> can use Phaser instead of Semaphore and make the code a further bit more
> readable.

CyclicBarrier is even simpler :)

Thanks for checking the "alive" status.

The setLive/isLive synchronization is going to cause a bit of a 
bottleneck, but that mainly means threads will be blocked on the monitor 
instead of sleeping, I think.

Minor style nits: for(int  -> for (int

Reviewed.

Thanks,
David

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