8023168: Cleanup LogManager class initialization and LogManager/LoggerContext relationship

8021003: java/util/logging/Logger/getGlobal/TestGetGlobalConcurrent.java fails intermittently
8019945: test/java/util/logging/LogManagerInstanceTest.java failing intermittently
Summary: This fix untangles the class initialization of Logger and LogManager, and also cleans up the relationship between LogManager, LoggerContext, and Logger, which were at the root cause of some intermittent test failures.
Reviewed-by: mchung, martin, plevart

src/share/classes/java/util/logging/Logger.java

@@ -245,14 +245,26 @@ public class Logger {
         // In order to finish the initialization of the global logger, we
         // will therefore call LogManager.getLogManager() here.
         //
-        // Care must be taken *not* to call Logger.getGlobal() in
-        // LogManager static initializers in order to avoid such
-        // deadlocks.
-        //
-        if (global != null && global.manager == null) {
-            // Complete initialization of the global Logger.
-            global.manager = LogManager.getLogManager();
-        }
+        // To prevent race conditions we also need to call
+        // LogManager.getLogManager() unconditionally here.
+        // Indeed we cannot rely on the observed value of global.manager,
+        // because global.manager will become not null somewhere during
+        // the initialization of LogManager.
+        // If two threads are calling getGlobal() concurrently, one thread
+        // will see global.manager null and call LogManager.getLogManager(),
+        // but the other thread could come in at a time when global.manager
+        // is already set although ensureLogManagerInitialized is not finished
+        // yet...
+        // Calling LogManager.getLogManager() unconditionally will fix that.
+
+        LogManager.getLogManager();
+
+        // Now the global LogManager should be initialized,
+        // and the global logger should have been added to
+        // it, unless we were called within the constructor of a LogManager
+        // subclass installed as LogManager, in which case global.manager
+        // would still be null, and global will be lazily initialized later on.
+
         return global;
     }
 
@@ -298,11 +310,11 @@ public class Logger {
      *             no corresponding resource can be found.
      */
     protected Logger(String name, String resourceBundleName) {
-        this(name, resourceBundleName, null);
+        this(name, resourceBundleName, null, LogManager.getLogManager());
     }
 
-    Logger(String name, String resourceBundleName, Class<?> caller) {
-        this.manager = LogManager.getLogManager();
+    Logger(String name, String resourceBundleName, Class<?> caller, LogManager manager) {
+        this.manager = manager;
         setupResourceInfo(resourceBundleName, caller);
         this.name = name;
         levelValue = Level.INFO.intValue();
@@ -332,8 +344,8 @@ public class Logger {
         levelValue = Level.INFO.intValue();
     }
 
-    // It is called from the LogManager.<clinit> to complete
-    // initialization of the global Logger.
+    // It is called from LoggerContext.addLocalLogger() when the logger
+    // is actually added to a LogManager.
     void setLogManager(LogManager manager) {
         this.manager = manager;
     }
@@ -558,7 +570,7 @@ public class Logger {
         // cleanup some Loggers that have been GC'ed
         manager.drainLoggerRefQueueBounded();
         Logger result = new Logger(null, resourceBundleName,
-                                   Reflection.getCallerClass());
+                                   Reflection.getCallerClass(), manager);
         result.anonymous = true;
         Logger root = manager.getLogger("");
         result.doSetParent(root);
@@ -1798,7 +1810,7 @@ public class Logger {
         if (parent == null) {
             throw new NullPointerException();
         }
-        manager.checkPermission();
+        checkPermission();
         doSetParent(parent);
     }
 

test/java/util/logging/Logger/getGlobal/TestGetGlobal.java

@@ -57,6 +57,12 @@ public class TestGetGlobal {
     }
 
     public static void main(String... args) {
+        final String manager = System.getProperty("java.util.logging.manager", null);
+
+        final String description = "TestGetGlobal"
+            + (System.getSecurityManager() == null ? " " :
+               " -Djava.security.manager ")
+            + (manager == null ? "" : "-Djava.util.logging.manager=" + manager);
 
         Logger.global.info(messages[0]); // at this point LogManager is not
              // initialized yet, so this message should not appear.
@@ -67,7 +73,9 @@ public class TestGetGlobal {
 
         final List<String> expected = Arrays.asList(Arrays.copyOfRange(messages, 1, messages.length));
         if (!testgetglobal.HandlerImpl.received.equals(expected)) {
-            throw new Error("Unexpected message list: "+testgetglobal.HandlerImpl.received+" vs "+ expected);
+            System.err.println("Test case failed: " + description);
+            throw new Error("Unexpected message list: "+testgetglobal.HandlerImpl.received+" vs "+ expected
+                            + "\n\t"+description);
         }
     }
 }

test/java/util/logging/Logger/getGlobal/TestGetGlobalConcurrent.java

@@ -22,17 +22,18 @@
  */
 import java.util.Arrays;
 import java.util.List;
+import java.util.logging.Level;
 import java.util.logging.Logger;
 
 /**
  * @test
- * @bug 7184195
- * @summary checks that java.util.logging.Logger.getGlobal().info() logs without configuration
+ * @bug 7184195 8021003
+ * @summary Test that the global logger can log with no configuration when accessed from multiple threads.
  * @build TestGetGlobalConcurrent testgetglobal.HandlerImpl testgetglobal.LogManagerImpl1 testgetglobal.LogManagerImpl2 testgetglobal.LogManagerImpl3 testgetglobal.BadLogManagerImpl testgetglobal.DummyLogManagerImpl
  * @run main/othervm/timeout=10 TestGetGlobalConcurrent
  * @run main/othervm/timeout=10/policy=policy -Djava.security.manager TestGetGlobalConcurrent
- * @run main/othervm/timeout=10 -Djava.util.logging.manager=testgetglobal.LogManagerImpl TestGetGlobalConcurrent
- * @run main/othervm/timeout=10/policy=policy -Djava.security.manager -Djava.util.logging.manager=testgetglobal.LogManagerImpl TestGetGlobalConcurrent
+ * @run main/othervm/timeout=10 -Djava.util.logging.manager=testgetglobal.LogManagerImpl1 TestGetGlobalConcurrent
+ * @run main/othervm/timeout=10/policy=policy -Djava.security.manager -Djava.util.logging.manager=testgetglobal.LogManagerImpl1 TestGetGlobalConcurrent
  * @run main/othervm/timeout=10 -Djava.util.logging.manager=testgetglobal.LogManagerImpl2 TestGetGlobalConcurrent
  * @run main/othervm/timeout=10/policy=policy -Djava.security.manager -Djava.util.logging.manager=testgetglobal.LogManagerImpl2 TestGetGlobalConcurrent
  * @run main/othervm/timeout=10 -Djava.util.logging.manager=testgetglobal.LogManagerImpl3 TestGetGlobalConcurrent
@@ -69,7 +70,6 @@ public class TestGetGlobalConcurrent {
              // initialize the LogManager - and thus this message should appear.
         Logger.global.info(messages[i+1]); // Now that the LogManager is
              // initialized, this message should appear too.
-
         final List<String> expected = Arrays.asList(Arrays.copyOfRange(messages, i, i+2));
         if (!testgetglobal.HandlerImpl.received.containsAll(expected)) {
             fail(new Error("Unexpected message list: "+testgetglobal.HandlerImpl.received+" vs "+ expected));
@@ -82,7 +82,6 @@ public class TestGetGlobalConcurrent {
              // initialize the LogManager - and thus this message should appear.
         Logger.global.info(messages[i+1]); // Now that the LogManager is
              // initialized, this message should appear too.
-
         final List<String> expected = Arrays.asList(Arrays.copyOfRange(messages, i, i+2));
         if (!testgetglobal.HandlerImpl.received.containsAll(expected)) {
             fail(new Error("Unexpected message list: "+testgetglobal.HandlerImpl.received+" vs "+ expected));
@@ -96,7 +95,6 @@ public class TestGetGlobalConcurrent {
              // initialize the LogManager - and thus this message should appear.
         Logger.global.info(messages[i+1]); // Now that the LogManager is
              // initialized, this message should appear too.
-
         final List<String> expected = Arrays.asList(Arrays.copyOfRange(messages, i, i+2));
         if (!testgetglobal.HandlerImpl.received.containsAll(expected)) {
             fail(new Error("Unexpected message list: "+testgetglobal.HandlerImpl.received+" vs "+ expected));
@@ -150,8 +148,17 @@ public class TestGetGlobalConcurrent {
         public void run() { test4(); }
     }
 
+    static String description = "Unknown";
+
     public static void main(String... args) throws Exception {
 
+        final String manager = System.getProperty("java.util.logging.manager", null);
+
+        description = "TestGetGlobalConcurrent"
+            + (System.getSecurityManager() == null ? " " :
+               " -Djava.security.manager ")
+            + (manager == null ? "" : "-Djava.util.logging.manager=" + manager);
+
         final Thread t1 = new Thread(new WaitAndRun(new Run1()), "test1");
         final Thread t2 = new Thread(new WaitAndRun(new Run2()), "test2");
         final Thread t3 = new Thread(new WaitAndRun(new Run3()), "test3");
@@ -169,14 +176,13 @@ public class TestGetGlobalConcurrent {
 
         final List<String> expected = Arrays.asList(Arrays.copyOfRange(messages, 1, 3));
         if (!testgetglobal.HandlerImpl.received.containsAll(expected)) {
-            throw new Error("Unexpected message list: "+testgetglobal.HandlerImpl.received+" vs "+ expected);
+            fail(new Error("Unexpected message list: "+testgetglobal.HandlerImpl.received+" vs "+ expected));
         }
 
-
         t1.join(); t2.join(); t3.join(); t4.join();
 
         if (failed != null) {
-             throw new Error("Test failed.", failed);
+             throw new Error("Test failed: "+description, failed);
         }
 
         System.out.println("Test passed");

test/java/util/logging/Logger/getGlobal/policy

 grant {
     permission java.util.PropertyPermission "java.util.logging.config.file", "write";
     permission java.util.PropertyPermission "test.src", "read";
+    permission java.util.PropertyPermission "java.util.logging.manager", "read";
     permission java.lang.RuntimePermission "setContextClassLoader";
     permission java.lang.RuntimePermission "shutdownHooks";
     permission java.util.logging.LoggingPermission "control";

test/java/util/logging/ParentLoggersTest.java

@@ -29,7 +29,7 @@
  * @author  ss45998
  *
  * @build ParentLoggersTest
- * @run main/othervm ParentLoggersTest
+ * @run main ParentLoggersTest
  */
 
 /*

test/java/util/logging/TestAppletLoggerContext.java

@@ -38,7 +38,7 @@ import sun.misc.SharedSecrets;
 
 /*
  * @test
- * @bug 8017174 8010727
+ * @bug 8017174 8010727 8019945
  * @summary  NPE when using Logger.getAnonymousLogger or
  *           LogManager.getLogManager().getLogger
  *
@@ -432,45 +432,36 @@ public class TestAppletLoggerContext {
             assertNull(manager.getLogger(""));
             assertNull(manager.getLogger(""));
 
-            Bridge.changeContext();
-
-            // this is not a supported configuration:
-            // We are in an applet context with several log managers.
-            // We however need to check our assumptions...
-
-            // Applet context => root logger and global logger are not null.
-            //   root == LogManager.getLogManager().rootLogger
-            //   global == Logger.global
-
-            Logger logger3 = manager.getLogger(Logger.GLOBAL_LOGGER_NAME);
-            Logger logger3b = manager.getLogger(Logger.GLOBAL_LOGGER_NAME);
-            assertNotNull(logger3);
-            assertNotNull(logger3b);
-            Logger expected = (System.getSecurityManager() != null
-                  ? Logger.getGlobal()
-                  : global);
-            assertEquals(logger3, expected); // in applet context, we will not see
-                  // the LogManager's custom global logger added above...
-            assertEquals(logger3b, expected); // in applet context, we will not see
-                  // the LogManager's custom global logger added above...
-            Logger global2 = new Bridge.CustomLogger(Logger.GLOBAL_LOGGER_NAME);
-            manager.addLogger(global2); // adding a global logger will not work in applet context
-               // we will always get back the global logger.
-               // this could be considered as a bug...
-            Logger logger4 = manager.getLogger(Logger.GLOBAL_LOGGER_NAME);
-            Logger logger4b = manager.getLogger(Logger.GLOBAL_LOGGER_NAME);
-            assertNotNull(logger4);
-            assertNotNull(logger4b);
-            assertEquals(logger4,  expected); // adding a global logger will not work in applet context
-            assertEquals(logger4b, expected); // adding a global logger will not work in applet context
-
-            Logger logger5 = manager.getLogger("");
-            Logger logger5b = manager.getLogger("");
-            Logger expectedRoot = (System.getSecurityManager() != null
-                  ? LogManager.getLogManager().getLogger("")
-                  : null);
-            assertEquals(logger5, expectedRoot);
-            assertEquals(logger5b, expectedRoot);
+            for (int j = 0; j<3; j++) {
+                Bridge.changeContext();
+
+                // this is not a supported configuration:
+                // We are in an applet context with several log managers.
+                // We however need to check our assumptions...
+
+                // Applet context => root logger and global logger should also be null.
+
+                Logger expected = (System.getSecurityManager() == null ? global : null);
+                Logger logger3 = manager.getLogger(Logger.GLOBAL_LOGGER_NAME);
+                Logger logger3b = manager.getLogger(Logger.GLOBAL_LOGGER_NAME);
+                assertEquals(expected, logger3);
+                assertEquals(expected, logger3b);
+                Logger global2 = new Bridge.CustomLogger(Logger.GLOBAL_LOGGER_NAME);
+                manager.addLogger(global2);
+                Logger logger4 = manager.getLogger(Logger.GLOBAL_LOGGER_NAME);
+                Logger logger4b = manager.getLogger(Logger.GLOBAL_LOGGER_NAME);
+                assertNotNull(logger4);
+                assertNotNull(logger4b);
+                expected = (System.getSecurityManager() == null ? global : global2);;
+                assertEquals(logger4,  expected);
+                assertEquals(logger4b, expected);
+
+                Logger logger5 = manager.getLogger("");
+                Logger logger5b = manager.getLogger("");
+                Logger expectedRoot = null;
+                assertEquals(logger5, expectedRoot);
+                assertEquals(logger5b, expectedRoot);
+            }
 
         }
     }
@@ -511,57 +502,53 @@ public class TestAppletLoggerContext {
             assertEquals(logger4, root);
             assertEquals(logger4b, root);
 
-            Bridge.changeContext();
+            for (int j = 0 ; j < 3 ; j++) {
+                Bridge.changeContext();
 
-            // this is not a supported configuration:
-            // We are in an applet context with several log managers.
-            // We haowever need to check our assumptions...
-
-            // Applet context => root logger and global logger are not null.
-            //   root == LogManager.getLogManager().rootLogger
-            //   global == Logger.global
-
-            Logger logger5 = manager.getLogger("");
-            Logger logger5b = manager.getLogger("");
-            Logger expectedRoot = (System.getSecurityManager() != null
-                  ? LogManager.getLogManager().getLogger("")
-                  : root);
-
-            assertNotNull(logger5);
-            assertNotNull(logger5b);
-            assertEquals(logger5, expectedRoot);
-            assertEquals(logger5b, expectedRoot);
-            if (System.getSecurityManager() != null) {
-                assertNotEquals(logger5, root);
-                assertNotEquals(logger5b, root);
-            }
+                // this is not a supported configuration:
+                // We are in an applet context with several log managers.
+                // We however need to check our assumptions...
 
-            Logger global2 = new Bridge.CustomLogger(Logger.GLOBAL_LOGGER_NAME);
-            manager.addLogger(global2); // adding a global logger will not work in applet context
-               // we will always get back the global logger.
-               // this could be considered as a bug...
-            Logger logger6 = manager.getLogger(Logger.GLOBAL_LOGGER_NAME);
-            Logger logger6b = manager.getLogger(Logger.GLOBAL_LOGGER_NAME);
-            Logger expectedGlobal = (System.getSecurityManager() != null
-                  ? Logger.getGlobal()
-                  : global);
-            assertNotNull(logger6);
-            assertNotNull(logger6b);
-            assertEquals(logger6, expectedGlobal); // adding a global logger will not work in applet context
-            assertEquals(logger6b, expectedGlobal); // adding a global logger will not work in applet context
-
-            Logger root2 = new Bridge.CustomLogger("");
-            manager.addLogger(root2); // adding a root logger will not work in applet context
-               // we will always get back the default manager's root logger.
-               // this could be considered as a bug...
-            Logger logger7 = manager.getLogger("");
-            Logger logger7b = manager.getLogger("");
-            assertNotNull(logger7);
-            assertNotNull(logger7b);
-            assertEquals(logger7, expectedRoot); // adding a global logger will not work in applet context
-            assertEquals(logger7b, expectedRoot); // adding a global logger will not work in applet context
-            assertNotEquals(logger7, root2);
-            assertNotEquals(logger7b, root2);
+                // Applet context => root logger and global logger should also be null.
+
+                Logger logger5 = manager.getLogger("");
+                Logger logger5b = manager.getLogger("");
+                Logger expectedRoot = (System.getSecurityManager() == null ? root : null);
+                assertEquals(logger5, expectedRoot);
+                assertEquals(logger5b, expectedRoot);
+
+                if (System.getSecurityManager() != null) {
+                    assertNull(manager.getLogger(Logger.GLOBAL_LOGGER_NAME));
+                } else {
+                    assertEquals(global, manager.getLogger(Logger.GLOBAL_LOGGER_NAME));
+                }
+
+                Logger global2 = new Bridge.CustomLogger(Logger.GLOBAL_LOGGER_NAME);
+                manager.addLogger(global2);
+                Logger logger6 = manager.getLogger(Logger.GLOBAL_LOGGER_NAME);
+                Logger logger6b = manager.getLogger(Logger.GLOBAL_LOGGER_NAME);
+                Logger expectedGlobal = (System.getSecurityManager() == null ? global : global2);
+
+                assertNotNull(logger6);
+                assertNotNull(logger6b);
+                assertEquals(logger6, expectedGlobal);
+                assertEquals(logger6b, expectedGlobal);
+                if (System.getSecurityManager() != null) {
+                    assertNull(manager.getLogger(""));
+                } else {
+                    assertEquals(root, manager.getLogger(""));
+                }
+
+                Logger root2 = new Bridge.CustomLogger("");
+                manager.addLogger(root2);
+                expectedRoot = (System.getSecurityManager() == null ? root : root2);
+                Logger logger7 = manager.getLogger("");
+                Logger logger7b = manager.getLogger("");
+                assertNotNull(logger7);
+                assertNotNull(logger7b);
+                assertEquals(logger7, expectedRoot);
+                assertEquals(logger7b, expectedRoot);
+            }
         }
     }