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

[Gary Adams]

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