Martin, if I exclude the test can we push the change then? -Pavel
On 14 May 2014, at 22:10, Martin Buchholz <marti...@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.ra...@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 <marti...@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.dui...@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.ra...@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 > > > > > >
