Code Review - 6957683 TEST_BUG: test/java/util/concurrent/ThreadPoolExecutor/Custom.java failing

[David Holmes]

Thumbs up.

Thanks,
David
-----

On 23/11/2011 1:57 AM, Gary Adams wrote:
> Revised webrev is available :
> - removed the latch logic not required for main to worker thread
> coordination
> - increased the timeout to allow service shutdown to 120 seconds
> (will exit sooner if services finish)
> - increased delay after service shutdown to ensure
> thread counts will be updated.
>
> http://cr.openjdk.java.net/~gadams/6957683/
>
> On 11/21/11 09:33 PM, David Holmes wrote:
>> Gary,
>>
>> On 22/11/2011 6:26 AM, Gary Adams wrote:
>>> Not quite as sure about the internal timeouts in this regression test.
>>>
>>> Here's a proposed change that cranks up the timeouts. Since the test
>>> harness defaults to 2 minutes there's no reason the service termination
>>> should timeout more quickly. For the thread startup, I added a countdown
>>> latch so the main thread waits til the worker threads have started.
>>>
>>> Also, after the service termination completes increased the delay to
>>> ensure the thread count check will be accurate.
>>>
>>> http://cr.openjdk.java.net/~gadams/6957683/
>>
>> Sorry but I think my initial analysis of this problem was incorrect.
>> The initial test failure reported 3 failed cases:
>>
>> 1 not equal to 0
>> 11 not equal to 10
>> 1 not equal to 0
>>
>> which corresponded to failures at the marked lines:
>>
>> public static void main(String[] args) throws Throwable {
>> CustomTPE tpe = new CustomTPE();
>> equal(tpe.getCorePoolSize(), threadCount);
>> equal(countExecutorThreads(), 0);
>> for (int i = 0; i < threadCount; i++)
>> tpe.submit(new Runnable() { public void run() {}});
>> equal(countExecutorThreads(), threadCount);
>> equal(CustomTask.births.get(), threadCount);
>> tpe.shutdown();
>> tpe.awaitTermination(Long.MAX_VALUE, TimeUnit.NANOSECONDS);
>> Thread.sleep(10);
>> /*1*/ equal(countExecutorThreads(), 0);
>>
>> CustomSTPE stpe = new CustomSTPE();
>> for (int i = 0; i < threadCount; i++)
>> stpe.submit(new Runnable() { public void run() {}});
>> equal(CustomSTPE.decorations.get(), threadCount);
>> /*2*/ equal(countExecutorThreads(), threadCount);
>> stpe.shutdown();
>> stpe.awaitTermination(Long.MAX_VALUE, TimeUnit.NANOSECONDS);
>> Thread.sleep(10);
>> /*3/ equal(countExecutorThreads(), 0);
>>
>> System.out.printf("%nPassed = %d, failed = %d%n%n", passed, failed);
>> if (failed > 0) throw new Exception("Some tests failed");
>> }
>>
>> However, I think the failure at /*2*/ is actually seeing the same
>> "extra" thread as reported at /*1*/. It could even be the same thread
>> again at /*3*/ but we have other independent failure cases for /*3*/.
>>
>> When we create the STPE each submit will create and start a worker
>> thread until we reach the core pool size. These worker threads will be
>> immediately visible to the countExecutorThreads() logic because it
>> simply enumerates the ThreadGroup. I was mistakenly thinking that
>> these threads need to actually get a chance to execute to become
>> visible to the counting logic, but that is not the case.
>>
>> Consequently you can elide the change to add the latch and just leave
>> the defensive timeouts on the awaitTermination with the increased
>> sleep delays to give the worker threads a chance to really terminate.
>>
>> Sorry for sending you down the wrong path on this one.
>>
>> David
>> -----
>