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

[Jaroslav Bachorik]

On 10.3.2015 00:35, David Holmes wrote:
> 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 :)

Yes, but the test threads don't need to wait for each other - 
Phaser.arrive() maps better here.

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

Exactly, the bottleneck is not a problem here.

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

Will fix before push.

Thanks,

-JB-

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