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-
