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

[Martin Buchholz]

Yes, please do that!


On Thu, May 15, 2014 at 5:53 AM, Pavel Rappo <pavel.rappo at oracle.com> wrote:

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