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

[Pavel Rappo]

Martin, if I exclude the test can we push the change then?

-Pavel

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

> We added the necessary support for jdk9+ tests and added the test below, which I think suffices.  I don't think a separate jtreg test is necessary.  (Just need to make sure openjdk testers also run Doug's jsr166 CVS tests!)
> 
> /*
>  * Written by Martin Buchholz and Doug Lea with assistance from
>  * members of JCP JSR-166 Expert Group and released to the public
>  * domain, as explained at
>  * http://creativecommons.org/publicdomain/zero/1.0/
>  */
> 
> import junit.framework.*;
> import java.util.concurrent.*;
> import static java.util.concurrent.TimeUnit.MILLISECONDS;
> import static java.util.concurrent.TimeUnit.NANOSECONDS;
> import java.util.*;
> 
> public class ThreadPoolExecutor9Test extends JSR166TestCase {
>     public static void main(String[] args) {
>         junit.textui.TestRunner.run(suite());
>     }
>     public static Test suite() {
>         return new TestSuite(ThreadPoolExecutor9Test.class);
>     }
> 
>     /**
>      * Configuration changes that allow core pool size greater than
>      * max pool size result in IllegalArgumentException.
>      */
>     public void testPoolSizeInvariants() {
>         ThreadPoolExecutor p =
>             new ThreadPoolExecutor(1, 1,
>                                    LONG_DELAY_MS, MILLISECONDS,
>                                    new ArrayBlockingQueue<Runnable>(10));
>         for (int s = 1; s < 5; s++) {
>             p.setMaximumPoolSize(s);
>             p.setCorePoolSize(s);
>             try {
>                 p.setMaximumPoolSize(s - 1);
>                 shouldThrow();
>             } catch (IllegalArgumentException success) {}
>             assertEquals(s, p.getCorePoolSize());
>             assertEquals(s, p.getMaximumPoolSize());
>             try {
>                 p.setCorePoolSize(s + 1);
>                 shouldThrow();
>             } catch (IllegalArgumentException success) {}
>             assertEquals(s, p.getCorePoolSize());
>             assertEquals(s, p.getMaximumPoolSize());
>         }
>         joinPool(p);
>     }
> 
> }
> 
> 
> 
> On Wed, May 14, 2014 at 2:02 PM, Pavel Rappo <pavel.rappo at oracle.com> wrote:
> 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
> >
> >
> 
>