RFR JDK-7153400: ThreadPoolExecutor's setCorePoolSize method allows corePoolSize > maxPoolSize

[Pavel Rappo]

Hi Martin,

Thanks for you comments. I forgot indeed that awaitTermination indicates its result by returning a boolean value rather than throwing TimeoutException. So this should be fine now:

@@ -77,7 +77,10 @@
     private static void dispose(ThreadPoolExecutor p) {
         p.shutdownNow();
         try {
-            p.awaitTermination(1, TimeUnit.MINUTES);
+            boolean shutdown = p.awaitTermination(1, TimeUnit.MINUTES);
+            if (!shutdown)
+                throw new RuntimeException(
+                        "Pool did not terminate in a timely manner");
         } catch (InterruptedException e) {
             throw new RuntimeException("Should not happen", e);
         }

As for the "fail" method, it's a little bit different from "assertThrows". I tried to keep my checks (test payload) to be one liners. So the whole lifecycle of a ThreadPoolExecutor is confined in a single line. In addition to check whether the IllegalArgumentException is thrown, "fail" also disposes the pool. It's not clean object oriented design, I agree, but it was done for the sake of clarity. This test is supposed to be simple.

-Pavel

On 14 May 2014, at 18:21, Martin Buchholz <martinrb at google.com> wrote:

> Pavel,
> 
> Thanks for writing a test.
> 
> We (jsr166 maintainers will add the jtreg test to jsr166 CVS when it has passed review.
> 
> Instead of "succeed", I would just write main-line code.  If you want per-api-call granularity, write a testng test.
> 
> Instead of "fail", I suggest as in jsr166 CVS src/test/tck/JSR166TestCase.java :
> 
>     public void assertThrows(Class<? extends Throwable> expectedExceptionClass,
>                              Runnable... throwingActions) {
>         for (Runnable throwingAction : throwingActions) {
>             boolean threw = false;
>             try { throwingAction.run(); }
>             catch (Throwable t) {
>                 threw = true;
>                 if (!expectedExceptionClass.isInstance(t)) {
>                     AssertionFailedError afe =
>                         new AssertionFailedError
>                         ("Expected " + expectedExceptionClass.getName() +
>                          ", got " + t.getClass().getName());
>                     afe.initCause(t);
>                     threadUnexpectedException(afe);
>                 }
>             }
>             if (!threw)
>                 shouldThrow(expectedExceptionClass.getName());
>         }
>     }
> 
> I suggest checking the return from p.awaitTermination
>             p.awaitTermination(1, TimeUnit.MINUTES);
> 
> as in src/test/tck/JSR166TestCase.java:
> 
> 
>     /**
>      * Waits out termination of a thread pool or fails doing so.
>      */
>     void joinPool(ExecutorService exec) {
>         try {
>             exec.shutdown();
>             assertTrue("ExecutorService did not terminate in a timely manner",
>                        exec.awaitTermination(2 * LONG_DELAY_MS, MILLISECONDS));
>         } catch (SecurityException ok) {
>             // Allowed in case test doesn't have privs
>         } catch (InterruptedException ie) {
>             fail("Unexpected InterruptedException");
>         }
>     }
> 
> 
> 
> 
> On Wed, May 14, 2014 at 10:03 AM, Mike Duigou <mike.duigou at oracle.com> wrote:
> Hi Pavel;
> 
> The change and test looks good. Will the test be upstreamed or will Doug be adding a similar test in his upstream?
> 
> Mike
> 
> On May 14 2014, at 08:29 , Pavel Rappo <pavel.rappo at oracle.com> wrote:
> 
> > Hi everyone,
> >
> > could you please review my change for JDK-7153400?
> >
> > http://cr.openjdk.java.net/~chegar/7153400/00/webrev/
> > http://ccc.us.oracle.com/7153400
> >
> > It's a long expected fix for a minor issue in the ThreadPoolExecutor. This has been agreed with Doug Lea. The exact same change (except for the test) is already in jsr166 repo: http://gee.cs.oswego.edu/cgi-bin/viewcvs.cgi/jsr166/src/main/java/util/concurrent/ThreadPoolExecutor.java?r1=1.151&r2=1.152
> >
> > Thanks
> > -Pavel
> 
>