I've put the patch up on http://cr.openjdk.java.net/~martin/LoggingDeadlock2
(my first use (or abuse? I didn't use webrev) of cr.openjdk.java.net) I am not a shutdown hook expert - everything I know about shutdown hooks I learned while tracking down LoggingDeadlock2 failures. So I agree it would be good for you (Mandy) to pick this up. One way to separate the work would be for you to file a bug for bug #1, I check in only the fix for the test case for bug #1, intentionally leaving it still flaky, but serving as a regression test for your followon fix for bug #2. You just need to add the @bug for bug #2 to my test. Deal? Martin On Mon, Apr 13, 2009 at 10:56, Mandy Chung <[email protected]> wrote: > Hi Martin, > > Thanks for catching the regression. I created a Sun bug # 6829503 for bug > 2. > > With the fix for 6829503, ApplicationShutdownHook and DeleteOnExitHook are > both lazily initialized. Since it is possible that a file could be added to > the list for DeletedOnExit during shutdown, perhaps we also need to change > the Shutdown.add method to allow DeleteOnExitHook to be added during > shutdown. > I should pick up this bug since my fix caused this regression. Or do you > want to revise the patch? > > Thanks > Mandy > > Martin Buchholz wrote: >> >> Hi ShutdownHooks/Logging maintainers (especially Mandy), >> >> This is a bug report for *two* bugs, with fix. >> >> Bug 1: >> Synopsis: test/java/util/logging/LoggingDeadlock2.java is flaky >> Description: >> The reg test java/util/logging/LoggingDeadlock2.java >> has been failing intermittently for quite a while, at least since >> jdk7-b44. >> It calls System.exit, which a jtreg test is not allowed to do (at >> least not directly; >> it must run the test in a subprocess). jtreg notices this if the >> timing is just right. >> >> Evaluation: >> Yup. It's tedious to create a java subprocess, but we've done it >> before and we know how. >> >> Bug 2: >> Synopsis: addShutdownHook fails if called after shutdown has commenced. >> Description: >> If you call addShutdownHook after shutdown has already started, >> you should get IllegalStateException, as specified. >> If the call to addShutdownHook is the first one in this JDK, >> you will get an ExceptionInInitializerError instead, as below >> >> Exception in thread "main" java.lang.ExceptionInInitializerError >> at java.lang.Runtime.addShutdownHook(Runtime.java:209) >> at java.util.logging.LogManager.<init>(LogManager.java:246) >> at java.util.logging.LogManager$1.run(LogManager.java:194) >> at java.security.AccessController.doPrivileged(Native Method) >> at java.util.logging.LogManager.<clinit>(LogManager.java:175) >> at LoggingDeadlock2$JavaChild.main(LoggingDeadlock2.java:106) >> Caused by: java.lang.IllegalStateException: Shutdown in progress >> at java.lang.Shutdown.add(Shutdown.java:77) >> at >> java.lang.ApplicationShutdownHooks.<clinit>(ApplicationShutdownHooks.java:39) >> >> This appears to be due to changes in >> >> # HG changeset patch >> # User mchung >> # Date 1236878842 25200 >> # Node ID e1064300e0f6948e3ba820fab7a4040beeed930c >> # Parent aa48deaf9af44bffce04a80ea1ea6a61901fd286 >> 6810254: Lazily instantiate the shared secret access objects >> Summary: Register the shutdown hooks only when needed and remove >> JavaIODeleteOnExitAccess >> Reviewed-by: alanb >> >> --- a/src/share/classes/java/lang/ApplicationShutdownHooks.java >> +++ b/src/share/classes/java/lang/ApplicationShutdownHooks.java >> @@ -34,18 +34,18 @@ >> * @see java.lang.Runtime#removeShutdownHook >> */ >> >> -class ApplicationShutdownHooks implements Runnable { >> - private static ApplicationShutdownHooks instance = null; >> +class ApplicationShutdownHooks { >> + static { >> + Shutdown.add(1 /* shutdown hook invocation order */, >> + new Runnable() { >> + public void run() { >> + runHooks(); >> + } >> + }); >> + } >> >> Evaluation: >> ApplicationShutdownHooks.<clinit> needs to handle ISE from >> Shutdown.add >> >> PATCH: (The rewritten test is a regtest for bug 2) >> >> # HG changeset patch >> # User martin >> # Date 1239567519 25200 >> # Node ID 8603336c1cfcb914cbb41deb6a2c8e141e96a803 >> # Parent 22b6e09960c153788717121f24ede64d845e8095 >> [mq]: LoggingDeadlock2 >> >> diff --git a/src/share/classes/java/lang/ApplicationShutdownHooks.java >> b/src/share/classes/java/lang/ApplicationShutdownHooks.java >> --- a/src/share/classes/java/lang/ApplicationShutdownHooks.java >> +++ b/src/share/classes/java/lang/ApplicationShutdownHooks.java >> @@ -26,34 +26,38 @@ >> >> import java.util.*; >> >> -/* >> +/** >> * Class to track and run user level shutdown hooks registered through >> * <tt>{...@link Runtime#addShutdownHook Runtime.addShutdownHook}</tt>. >> * >> * @see java.lang.Runtime#addShutdownHook >> * @see java.lang.Runtime#removeShutdownHook >> */ >> - >> class ApplicationShutdownHooks { >> static { >> - Shutdown.add(1 /* shutdown hook invocation order */, >> - new Runnable() { >> - public void run() { >> - runHooks(); >> - } >> - }); >> + try { >> + Shutdown.add(1 /* shutdown hook invocation order */, >> + new Runnable() { >> + public void run() { >> + runHooks(); >> + } >> + }); >> + } catch (IllegalStateException e) { >> + // shutdown already in progress >> + } >> } >> >> - /* The set of registered hooks */ >> + /** The set of registered hooks */ >> private static IdentityHashMap<Thread, Thread> hooks = new >> IdentityHashMap<Thread, Thread>(); >> >> private ApplicationShutdownHooks() {} >> >> - /* Add a new shutdown hook. Checks the shutdown state and the hook >> itself, >> + /** >> + * Adds a new shutdown hook. Checks the shutdown state and the >> hook itself, >> * but does not do any security checks. >> */ >> static synchronized void add(Thread hook) { >> - if(hooks == null) >> + if (hooks == null) >> throw new IllegalStateException("Shutdown in progress"); >> >> if (hook.isAlive()) >> @@ -65,11 +69,12 @@ >> hooks.put(hook, hook); >> } >> >> - /* Remove a previously-registered hook. Like the add method, this >> method >> + /** >> + * Removes a previously-registered hook. Like the add method, this >> method >> * does not do any security checks. >> */ >> static synchronized boolean remove(Thread hook) { >> - if(hooks == null) >> + if (hooks == null) >> throw new IllegalStateException("Shutdown in progress"); >> >> if (hook == null) >> @@ -78,7 +83,8 @@ >> return hooks.remove(hook) != null; >> } >> >> - /* Iterates over all application hooks creating a new thread for each >> + /** >> + * Iterates over all application hooks creating a new thread for each >> * to run in. Hooks are run concurrently and this method waits for >> * them to finish. >> */ >> diff --git a/test/java/util/logging/LoggingDeadlock2.java >> b/test/java/util/logging/LoggingDeadlock2.java >> --- a/test/java/util/logging/LoggingDeadlock2.java >> +++ b/test/java/util/logging/LoggingDeadlock2.java >> @@ -26,7 +26,7 @@ >> * @bug 6467152 >> * >> * @summary deadlock occurs in LogManager initialization and JVM >> termination >> - * @author Serguei Spitsyn / Hittachi >> + * @author Serguei Spitsyn / Hitachi >> * >> * @build LoggingDeadlock2 >> * @run main/timeout=15 LoggingDeadlock2 >> @@ -47,43 +47,195 @@ >> * This is a regression test for this bug. >> */ >> >> +import java.util.Arrays; >> +import java.util.List; >> +import java.util.Random; >> +import java.util.concurrent.CyclicBarrier; >> +import java.util.concurrent.atomic.AtomicInteger; >> import java.util.logging.LogManager; >> +import java.io.File; >> +import java.io.InputStream; >> +import java.io.InputStreamReader; >> +import java.io.Reader; >> >> -public class LoggingDeadlock2 implements Runnable { >> - static final java.io.PrintStream out = System.out; >> - static Object lock = new Object(); >> - static int c = 0; >> - public static void main(String arg[]) { >> - out.println("\nThis test checks that there is no deadlock."); >> - out.println("If not crashed or timed-out then it is passed."); >> +public class LoggingDeadlock2 { >> + >> + public static void realMain(String arg[]) throws Throwable { >> try { >> - new Thread(new LoggingDeadlock2()).start(); >> - synchronized(lock) { >> - c++; >> - if (c == 2) lock.notify(); >> - else lock.wait(); >> + System.out.println(javaChildArgs); >> + ProcessBuilder pb = new ProcessBuilder(javaChildArgs); >> + ProcessResults r = run(pb.start()); >> + equal(r.exitValue(), 99); >> + equal(r.out(), ""); >> + equal(r.err(), ""); >> + } catch (Throwable t) { unexpected(t); } >> + } >> + >> + public static class JavaChild { >> + public static void main(String args[]) throws Throwable { >> + final CyclicBarrier startingGate = new CyclicBarrier(2); >> + final Throwable[] thrown = new Throwable[1]; >> + >> + // Some random variation, to help tickle races. >> + final Random rnd = new Random(); >> + final boolean dojoin = rnd.nextBoolean(); >> + final int JITTER = 1024; >> + final int iters1 = rnd.nextInt(JITTER); >> + final int iters2 = JITTER - iters1; >> + final AtomicInteger counter = new AtomicInteger(0); >> + >> + Thread exiter = new Thread() { >> + public void run() { >> + try { >> + startingGate.await(); >> + for (int i = 0; i < iters1; i++) >> + counter.getAndIncrement(); >> + System.exit(99); >> + } catch (Throwable t) { >> + t.printStackTrace(); >> + System.exit(86); >> + } >> + }}; >> + exiter.start(); >> + >> + startingGate.await(); >> + for (int i = 0; i < iters2; i++) >> + counter.getAndIncrement(); >> + // This may or may not result in a first call to >> + // Runtime.addShutdownHook after shutdown has already >> + // commenced. >> + LogManager log = LogManager.getLogManager(); >> + >> + if (dojoin) { >> + exiter.join(); >> + if (thrown[0] != null) >> + throw new Error(thrown[0]); >> + check(counter.get() == JITTER); >> } >> - LogManager log = LogManager.getLogManager(); >> - out.println("Test passed"); >> - } >> - catch(Exception e) { >> - e.printStackTrace(); >> - out.println("Test FAILED"); // Not expected >> } >> } >> >> - public void run() { >> - try { >> - synchronized(lock) { >> - c++; >> - if (c == 2) lock.notify(); >> - else lock.wait(); >> - } >> - System.exit(1); >> + //---------------------------------------------------------------- >> + // The rest of this test is copied from ProcessBuilder/Basic.java >> + //---------------------------------------------------------------- >> + private static final String javaExe = >> + System.getProperty("java.home") + >> + File.separator + "bin" + File.separator + "java"; >> + >> + private static final String classpath = >> + System.getProperty("java.class.path"); >> + >> + private static final List<String> javaChildArgs = >> + Arrays.asList(new String[] >> + { javaExe, "-classpath", classpath, >> + "LoggingDeadlock2$JavaChild"}); >> + >> + private static class ProcessResults { >> + private final String out; >> + private final String err; >> + private final int exitValue; >> + private final Throwable throwable; >> + >> + public ProcessResults(String out, >> + String err, >> + int exitValue, >> + Throwable throwable) { >> + this.out = out; >> + this.err = err; >> + this.exitValue = exitValue; >> + this.throwable = throwable; >> } >> - catch(Exception e) { >> - e.printStackTrace(); >> - out.println("Test FAILED"); // Not expected >> + >> + public String out() { return out; } >> + public String err() { return err; } >> + public int exitValue() { return exitValue; } >> + public Throwable throwable() { return throwable; } >> + >> + public String toString() { >> + StringBuilder sb = new StringBuilder(); >> + sb.append("<STDOUT>\n" + out() + "</STDOUT>\n") >> + .append("<STDERR>\n" + err() + "</STDERR>\n") >> + .append("exitValue = " + exitValue + "\n"); >> + if (throwable != null) >> + sb.append(throwable.getStackTrace()); >> + return sb.toString(); >> } >> } >> + >> + private static class StreamAccumulator extends Thread { >> + private final InputStream is; >> + private final StringBuilder sb = new StringBuilder(); >> + private Throwable throwable = null; >> + >> + public String result () throws Throwable { >> + if (throwable != null) >> + throw throwable; >> + return sb.toString(); >> + } >> + >> + StreamAccumulator (InputStream is) { >> + this.is = is; >> + } >> + >> + public void run() { >> + try { >> + Reader r = new InputStreamReader(is); >> + char[] buf = new char[4096]; >> + int n; >> + while ((n = r.read(buf)) > 0) { >> + sb.append(buf,0,n); >> + } >> + } catch (Throwable t) { >> + throwable = t; >> + } finally { >> + try { is.close(); } >> + catch (Throwable t) { throwable = t; } >> + } >> + } >> + } >> + >> + private static ProcessResults run(Process p) { >> + Throwable throwable = null; >> + int exitValue = -1; >> + String out = ""; >> + String err = ""; >> + >> + StreamAccumulator outAccumulator = >> + new StreamAccumulator(p.getInputStream()); >> + StreamAccumulator errAccumulator = >> + new StreamAccumulator(p.getErrorStream()); >> + >> + try { >> + outAccumulator.start(); >> + errAccumulator.start(); >> + >> + exitValue = p.waitFor(); >> + >> + outAccumulator.join(); >> + errAccumulator.join(); >> + >> + out = outAccumulator.result(); >> + err = errAccumulator.result(); >> + } catch (Throwable t) { >> + throwable = t; >> + } >> + >> + return new ProcessResults(out, err, exitValue, throwable); >> + } >> + >> + //--------------------- Infrastructure --------------------------- >> + static volatile int passed = 0, failed = 0; >> + static void pass() {passed++;} >> + static void fail() {failed++; Thread.dumpStack();} >> + static void fail(String msg) {System.out.println(msg); fail();} >> + static void unexpected(Throwable t) {failed++; t.printStackTrace();} >> + static void check(boolean cond) {if (cond) pass(); else fail();} >> + static void check(boolean cond, String m) {if (cond) pass(); else >> fail(m);} >> + static void equal(Object x, Object y) { >> + if (x == null ? y == null : x.equals(y)) pass(); >> + else fail(x + " not equal to " + y);} >> + public static void main(String[] args) throws Throwable { >> + try {realMain(args);} catch (Throwable t) {unexpected(t);} >> + System.out.printf("%nPassed = %d, failed = %d%n%n", passed, >> failed); >> + if (failed > 0) throw new AssertionError("Some tests failed");} >> } >> > >
