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