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

[Martin Buchholz]

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