OpenJDK / jdk / hs
changeset 29740:d9f64fdd3c97
7113878: LogManager - namedLoggers should be ConcurrentHashMap instead of Hashtable
Summary: namedLoggers is now a ConcurrentHashMap. findLogger is updated to take benefit of the change.
Reviewed-by: dholmes, lancea, martin, mchung, plevart
Contributed-by: Peter Levart <peter.levart@gmail.com>, Daniel Fuchs <daniel.fuchs@oracle.com>
author | dfuchs |
---|---|
date | Thu, 02 Apr 2015 16:24:46 +0200 |
parents | 3966f5f6a6fd |
children | da2598cb299e |
files | jdk/src/java.logging/share/classes/java/util/logging/LogManager.java jdk/test/java/util/logging/LogManager/TestLoggerNames.java |
diffstat | 2 files changed, 286 insertions(+), 16 deletions(-) [+] |
line wrap: on
line diff
--- a/jdk/src/java.logging/share/classes/java/util/logging/LogManager.java Thu Apr 02 11:42:07 2015 +0200 +++ b/jdk/src/java.logging/share/classes/java/util/logging/LogManager.java Thu Apr 02 16:24:46 2015 +0200 @@ -31,6 +31,7 @@ import java.security.*; import java.lang.ref.ReferenceQueue; import java.lang.ref.WeakReference; +import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.CopyOnWriteArrayList; import sun.misc.JavaAWTAccess; import sun.misc.SharedSecrets; @@ -579,7 +580,8 @@ // added in the user context. class LoggerContext { // Table of named Loggers that maps names to Loggers. - private final Hashtable<String,LoggerWeakRef> namedLoggers = new Hashtable<>(); + private final ConcurrentHashMap<String,LoggerWeakRef> namedLoggers = + new ConcurrentHashMap<>(); // Tree of named Loggers private final LogNode root; private LoggerContext() { @@ -642,21 +644,44 @@ } - synchronized Logger findLogger(String name) { - // ensure that this context is properly initialized before - // looking for loggers. - ensureInitialized(); + Logger findLogger(String name) { + // Attempt to find logger without locking. LoggerWeakRef ref = namedLoggers.get(name); - if (ref == null) { - return null; + Logger logger = ref == null ? null : ref.get(); + + // if logger is not null, then we can return it right away. + // if name is "" or "global" and logger is null + // we need to fall through and check that this context is + // initialized. + // if ref is not null and logger is null we also need to + // fall through. + if (logger != null || (ref == null && !name.isEmpty() + && !name.equals(Logger.GLOBAL_LOGGER_NAME))) { + return logger; } - Logger logger = ref.get(); - if (logger == null) { - // Hashtable holds stale weak reference - // to a logger which has been GC-ed. - ref.dispose(); + + // We either found a stale reference, or we were looking for + // "" or "global" and didn't find them. + // Make sure context is initialized (has the default loggers), + // and look up again, cleaning the stale reference if it hasn't + // been cleaned up in between. All this needs to be done inside + // a synchronized block. + synchronized(this) { + // ensure that this context is properly initialized before + // looking for loggers. + ensureInitialized(); + ref = namedLoggers.get(name); + if (ref == null) { + return null; + } + logger = ref.get(); + if (logger == null) { + // The namedLoggers map holds stale weak reference + // to a logger which has been GC-ed. + ref.dispose(); + } + return logger; } - return logger; } // This method is called before adding a logger to the @@ -752,7 +777,6 @@ final LogManager owner = getOwner(); logger.setLogManager(owner); ref = owner.new LoggerWeakRef(logger); - namedLoggers.put(name, ref); // Apply any initial level defined for the new logger, unless // the logger's level is already initialized @@ -789,10 +813,17 @@ node.walkAndSetParent(logger); // new LogNode is ready so tell the LoggerWeakRef about it ref.setNode(node); + + // Do not publish 'ref' in namedLoggers before the logger tree + // is fully updated - because the named logger will be visible as + // soon as it is published in namedLoggers (findLogger takes + // benefit of the ConcurrentHashMap implementation of namedLoggers + // to avoid synchronizing on retrieval when that is possible). + namedLoggers.put(name, ref); return true; } - synchronized void removeLoggerRef(String name, LoggerWeakRef ref) { + void removeLoggerRef(String name, LoggerWeakRef ref) { namedLoggers.remove(name, ref); } @@ -800,7 +831,7 @@ // ensure that this context is properly initialized before // returning logger names. ensureInitialized(); - return namedLoggers.keys(); + return Collections.enumeration(namedLoggers.keySet()); } // If logger.getUseParentHandlers() returns 'true' and any of the logger's
--- /dev/null Thu Jan 01 00:00:00 1970 +0000 +++ b/jdk/test/java/util/logging/LogManager/TestLoggerNames.java Thu Apr 02 16:24:46 2015 +0200 @@ -0,0 +1,239 @@ +/* + * Copyright (c) 2015, Oracle and/or its affiliates. All rights reserved. + * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. + * + * This code is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License version 2 only, as + * published by the Free Software Foundation. + * + * This code is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License + * version 2 for more details (a copy is included in the LICENSE file that + * accompanied this code). + * + * You should have received a copy of the GNU General Public License version + * 2 along with this work; if not, write to the Free Software Foundation, + * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA. + * + * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA + * or visit www.oracle.com if you need additional information or have any + * questions. + */ +import java.util.Collections; +import java.util.Enumeration; +import java.util.Iterator; +import java.util.List; +import java.util.concurrent.CopyOnWriteArrayList; +import java.util.concurrent.Phaser; +import java.util.concurrent.Semaphore; +import java.util.logging.Handler; +import java.util.logging.LogManager; +import java.util.logging.Logger; + + +/** + * @test + * @bug 7113878 + * @summary This is not a test that will check that 7113878 is fixed, but + * rather a test that will invoke the modified code & try to verify + * that fixing 7113878 has not introduced some big regression. + * This test should pass, whether 7113878 is there or not. + * @run main/othervm TestLoggerNames + * @author danielfuchs + */ +public class TestLoggerNames { + + static final class TestLogger extends java.util.logging.Logger { + + final Semaphore sem = new Semaphore(0); + final Semaphore wait = new Semaphore(0); + + public TestLogger(String name, String resourceBundleName) { + super(name, resourceBundleName); + } + + @Override + public Handler[] getHandlers() { + boolean found = false; + try { + System.out.println("Entering "+getName()+" getHandlers()"); + for (StackTraceElement ste : Thread.currentThread().getStackTrace()) { + if (LogManager.class.getName().equals(ste.getClassName()) + && "reset".equals(ste.getMethodName())) { + found = true; + System.out.println(getName()+" getHandlers() called by " + ste); + } + } + sem.release(); + try { + System.out.println("TestLogger: Acquiring wait for "+getName()); + wait.acquire(); + try { + System.out.println("TestLogger: Acquired wait for "+getName()); + return super.getHandlers(); + } finally { + System.out.println("TestLogger: Releasing wait for "+getName()); + wait.release(); + } + } finally { + System.out.println("Unblocking "+getName()); + sem.acquire(); + System.out.println("Unblocked "+getName()); + if (found) { + System.out.println("Reset will proceed..."); + } + } + } catch (InterruptedException x) { + throw new IllegalStateException(x); + } + } + } + + static volatile boolean stop; + static volatile Throwable resetFailed; + static volatile Throwable checkLoggerNamesFailed; + static volatile Phaser phaser = new Phaser(2); + + + static void checkLoggerNames(List<Logger> loggers) { + Enumeration<String> names = LogManager.getLogManager().getLoggerNames(); + if (names instanceof Iterator) { + for (Iterator<?> it = Iterator.class.cast(names); it.hasNext(); ) { + try { + it.remove(); + throw new RuntimeException("Iterator supports remove!"); + } catch (UnsupportedOperationException x) { + System.out.println("OK: Iterator doesn't support remove."); + } + } + } + List<String> loggerNames = Collections.list(names); + if (!loggerNames.contains("")) { + throw new RuntimeException("\"\"" + + " not found in " + loggerNames); + } + if (!loggerNames.contains("global")) { + throw new RuntimeException("global" + + " not found in " + loggerNames); + } + for (Logger l : loggers) { + if (!loggerNames.contains(l.getName())) { + throw new RuntimeException(l.getName() + + " not found in " + loggerNames); + } + } + System.out.println("Got all expected logger names"); + } + + + public static void main(String[] args) throws InterruptedException { + LogManager.getLogManager().addLogger(new TestLogger("com.foo.bar.zzz", null)); + try { + Logger.getLogger(null); + throw new RuntimeException("Logger.getLogger(null) didn't throw expected NPE"); + } catch (NullPointerException x) { + System.out.println("Got expected NullPointerException for Logger.getLogger(null)"); + } + List<Logger> loggers = new CopyOnWriteArrayList<>(); + loggers.add(Logger.getLogger("one.two.addMeAChild")); + loggers.add(Logger.getLogger("aaa.bbb.replaceMe")); + loggers.add(Logger.getLogger("bbb.aaa.addMeAChild")); + TestLogger test = (TestLogger)Logger.getLogger("com.foo.bar.zzz"); + loggers.add(Logger.getLogger("ddd.aaa.addMeAChild")); + + checkLoggerNames(loggers); + + Thread loggerNamesThread = new Thread(() -> { + try { + while (!stop) { + checkLoggerNames(loggers); + Thread.sleep(10); + if (!stop) { + phaser.arriveAndAwaitAdvance(); + } + } + } catch (Throwable t) { + t.printStackTrace(System.err); + checkLoggerNamesFailed = t; + } + }, "loggerNames"); + + Thread resetThread = new Thread(() -> { + try { + System.out.println("Calling reset..."); + LogManager.getLogManager().reset(); + System.out.println("Reset done..."); + System.out.println("Reset again..."); + LogManager.getLogManager().reset(); + System.out.println("Reset done..."); + } catch(Throwable t) { + resetFailed = t; + System.err.println("Unexpected exception or error in reset Thread"); + t.printStackTrace(System.err); + } + }, "reset"); + + resetThread.setDaemon(true); + resetThread.start(); + + System.out.println("Waiting for reset to get handlers"); + test.sem.acquire(); + try { + loggerNamesThread.start(); + System.out.println("Reset has called getHandlers on " + test.getName()); + int i = 0; + for (Enumeration<String> e = LogManager.getLogManager().getLoggerNames(); + e.hasMoreElements();) { + String name = e.nextElement(); + if (name.isEmpty()) continue; + if (name.endsWith(".addMeAChild")) { + Logger l = Logger.getLogger(name+".child"); + loggers.add(l); + System.out.println("*** Added " + l.getName()); + i++; + } else if (name.endsWith("replaceMe")) { + Logger l = Logger.getLogger(name); + loggers.remove(l); + l = Logger.getLogger(name.replace("replaceMe", "meReplaced")); + loggers.add(l); + System.gc(); + if (LogManager.getLogManager().getLogger(name) == null) { + System.out.println("*** "+ name + " successfully replaced with " + l.getName()); + } + i++; + } else { + System.out.println("Nothing to do for logger: " + name); + } + phaser.arriveAndAwaitAdvance(); + if (i >= 3 && i++ == 3) { + System.out.println("Loggers are now: " + + Collections.list(LogManager.getLogManager().getLoggerNames())); + test.wait.release(); + test.sem.release(); + System.out.println("Joining " + resetThread); + resetThread.join(); + } + } + } catch (RuntimeException | InterruptedException | Error x) { + test.wait.release(); + test.sem.release(); + throw x; + } finally { + stop = true; + phaser.arriveAndDeregister(); + loggerNamesThread.join(); + loggers.clear(); + } + + + if (resetFailed != null || checkLoggerNamesFailed != null) { + RuntimeException r = new RuntimeException("Some of the concurrent threads failed"); + if (resetFailed != null) r.addSuppressed(resetFailed); + if (checkLoggerNamesFailed != null) r.addSuppressed(checkLoggerNamesFailed); + throw r; + } + + } + +}