diff --git a/log4j-api/src/main/java/org/apache/logging/log4j/message/LocalizedMessageFactory.java b/log4j-api/src/main/java/org/apache/logging/log4j/message/LocalizedMessageFactory.java index d8360dff0e6..10d75fc9072 100644 --- a/log4j-api/src/main/java/org/apache/logging/log4j/message/LocalizedMessageFactory.java +++ b/log4j-api/src/main/java/org/apache/logging/log4j/message/LocalizedMessageFactory.java @@ -16,7 +16,9 @@ */ package org.apache.logging.log4j.message; +import java.util.Objects; import java.util.ResourceBundle; +import org.jspecify.annotations.Nullable; /** * Creates {@link FormattedMessage} instances for {@link MessageFactory2} methods (and {@link MessageFactory} by @@ -33,8 +35,11 @@ public class LocalizedMessageFactory extends AbstractMessageFactory { private static final long serialVersionUID = -1996295808703146741L; + @Nullable // FIXME: cannot use ResourceBundle name for serialization until Java 8 private final transient ResourceBundle resourceBundle; + + @Nullable private final String baseName; public LocalizedMessageFactory(final ResourceBundle resourceBundle) { @@ -92,4 +97,21 @@ public Message newMessage(final String key, final Object... params) { } return new LocalizedMessage(resourceBundle, key, params); } + + @Override + public boolean equals(final Object object) { + if (this == object) { + return true; + } + if (object == null || getClass() != object.getClass()) { + return false; + } + final LocalizedMessageFactory that = (LocalizedMessageFactory) object; + return Objects.equals(resourceBundle, that.resourceBundle) && Objects.equals(baseName, that.baseName); + } + + @Override + public int hashCode() { + return Objects.hash(resourceBundle, baseName); + } } diff --git a/log4j-api/src/main/java/org/apache/logging/log4j/message/package-info.java b/log4j-api/src/main/java/org/apache/logging/log4j/message/package-info.java index 816466b4675..dae91264f45 100644 --- a/log4j-api/src/main/java/org/apache/logging/log4j/message/package-info.java +++ b/log4j-api/src/main/java/org/apache/logging/log4j/message/package-info.java @@ -19,7 +19,7 @@ * Public Message Types used for Log4j 2. Users may implement their own Messages. */ @Export -@Version("2.24.0") +@Version("2.24.1") package org.apache.logging.log4j.message; import org.osgi.annotation.bundle.Export; diff --git a/log4j-api/src/main/java/org/apache/logging/log4j/simple/SimpleLoggerContext.java b/log4j-api/src/main/java/org/apache/logging/log4j/simple/SimpleLoggerContext.java index d1aaf8db0f7..7196b93ceb5 100644 --- a/log4j-api/src/main/java/org/apache/logging/log4j/simple/SimpleLoggerContext.java +++ b/log4j-api/src/main/java/org/apache/logging/log4j/simple/SimpleLoggerContext.java @@ -20,12 +20,13 @@ import java.io.PrintStream; import org.apache.logging.log4j.Level; import org.apache.logging.log4j.message.MessageFactory; +import org.apache.logging.log4j.message.ParameterizedMessageFactory; import org.apache.logging.log4j.simple.internal.SimpleProvider; -import org.apache.logging.log4j.spi.AbstractLogger; import org.apache.logging.log4j.spi.ExtendedLogger; import org.apache.logging.log4j.spi.LoggerContext; import org.apache.logging.log4j.spi.LoggerRegistry; import org.apache.logging.log4j.util.PropertiesUtil; +import org.jspecify.annotations.Nullable; /** * A simple {@link LoggerContext} implementation. @@ -41,6 +42,8 @@ public class SimpleLoggerContext implements LoggerContext { /** All system properties used by SimpleLog start with this */ protected static final String SYSTEM_PREFIX = "org.apache.logging.log4j.simplelog."; + private static final MessageFactory DEFAULT_MESSAGE_FACTORY = ParameterizedMessageFactory.INSTANCE; + private final PropertiesUtil props; /** Include the instance name in the log message? */ @@ -96,14 +99,20 @@ public ExtendedLogger getLogger(final String name) { } @Override - public ExtendedLogger getLogger(final String name, final MessageFactory messageFactory) { - // Note: This is the only method where we add entries to the 'loggerRegistry' ivar. - final ExtendedLogger extendedLogger = loggerRegistry.getLogger(name, messageFactory); - if (extendedLogger != null) { - AbstractLogger.checkMessageFactory(extendedLogger, messageFactory); - return extendedLogger; + public ExtendedLogger getLogger(final String name, @Nullable final MessageFactory messageFactory) { + final MessageFactory effectiveMessageFactory = + messageFactory != null ? messageFactory : DEFAULT_MESSAGE_FACTORY; + final ExtendedLogger oldLogger = loggerRegistry.getLogger(name, effectiveMessageFactory); + if (oldLogger != null) { + return oldLogger; } - final SimpleLogger simpleLogger = new SimpleLogger( + final ExtendedLogger newLogger = createLogger(name, effectiveMessageFactory); + loggerRegistry.putIfAbsent(name, effectiveMessageFactory, newLogger); + return loggerRegistry.getLogger(name, effectiveMessageFactory); + } + + private ExtendedLogger createLogger(final String name, @Nullable final MessageFactory messageFactory) { + return new SimpleLogger( name, defaultLevel, showLogName, @@ -114,8 +123,6 @@ public ExtendedLogger getLogger(final String name, final MessageFactory messageF messageFactory, props, stream); - loggerRegistry.putIfAbsent(name, messageFactory, simpleLogger); - return loggerRegistry.getLogger(name, messageFactory); } /** @@ -131,16 +138,18 @@ public LoggerRegistry getLoggerRegistry() { @Override public boolean hasLogger(final String name) { - return false; + return loggerRegistry.hasLogger(name, DEFAULT_MESSAGE_FACTORY); } @Override public boolean hasLogger(final String name, final Class messageFactoryClass) { - return false; + return loggerRegistry.hasLogger(name, messageFactoryClass); } @Override - public boolean hasLogger(final String name, final MessageFactory messageFactory) { - return false; + public boolean hasLogger(final String name, @Nullable final MessageFactory messageFactory) { + final MessageFactory effectiveMessageFactory = + messageFactory != null ? messageFactory : DEFAULT_MESSAGE_FACTORY; + return loggerRegistry.hasLogger(name, effectiveMessageFactory); } } diff --git a/log4j-api/src/main/java/org/apache/logging/log4j/simple/package-info.java b/log4j-api/src/main/java/org/apache/logging/log4j/simple/package-info.java index 1481df9462e..29139a12ace 100644 --- a/log4j-api/src/main/java/org/apache/logging/log4j/simple/package-info.java +++ b/log4j-api/src/main/java/org/apache/logging/log4j/simple/package-info.java @@ -20,7 +20,7 @@ * Providers are able to be loaded at runtime. */ @Export -@Version("2.24.0") +@Version("2.24.1") package org.apache.logging.log4j.simple; import org.osgi.annotation.bundle.Export; diff --git a/log4j-api/src/main/java/org/apache/logging/log4j/spi/LoggerRegistry.java b/log4j-api/src/main/java/org/apache/logging/log4j/spi/LoggerRegistry.java index e71c0a933db..4624757bb51 100644 --- a/log4j-api/src/main/java/org/apache/logging/log4j/spi/LoggerRegistry.java +++ b/log4j-api/src/main/java/org/apache/logging/log4j/spi/LoggerRegistry.java @@ -16,28 +16,43 @@ */ package org.apache.logging.log4j.spi; +import static java.util.Objects.requireNonNull; + +import java.lang.ref.WeakReference; import java.util.ArrayList; import java.util.Collection; +import java.util.Collections; +import java.util.HashMap; import java.util.Map; import java.util.Objects; import java.util.WeakHashMap; import java.util.concurrent.ConcurrentHashMap; -import java.util.concurrent.ConcurrentMap; +import java.util.concurrent.locks.Lock; +import java.util.concurrent.locks.ReadWriteLock; +import java.util.concurrent.locks.ReentrantReadWriteLock; import org.apache.logging.log4j.message.MessageFactory; +import org.apache.logging.log4j.message.ParameterizedMessageFactory; /** - * Convenience class to be used by {@code LoggerContext} implementations. + * Convenience class to be used as an {@link ExtendedLogger} registry by {@code LoggerContext} implementations. */ public class LoggerRegistry { - private static final String DEFAULT_FACTORY_KEY = AbstractLogger.DEFAULT_MESSAGE_FACTORY_CLASS.getName(); - private final MapFactory factory; - private final Map> map; + + private final Map>> loggerRefByMessageFactoryByName = new HashMap<>(); + + private final ReadWriteLock lock = new ReentrantReadWriteLock(); + + private final Lock readLock = lock.readLock(); + + private final Lock writeLock = lock.writeLock(); /** - * Interface to control the data structure used by the registry to store the Loggers. + * Data structure contract for the internal storage of admitted loggers. + * * @param subtype of {@code ExtendedLogger} */ public interface MapFactory { + Map createInnerMap(); Map> createOuterMap(); @@ -46,10 +61,12 @@ public interface MapFactory { } /** - * Generates ConcurrentHashMaps for use by the registry to store the Loggers. + * {@link MapFactory} implementation using {@link ConcurrentHashMap}. + * * @param subtype of {@code ExtendedLogger} */ public static class ConcurrentMapFactory implements MapFactory { + @Override public Map createInnerMap() { return new ConcurrentHashMap<>(); @@ -62,15 +79,17 @@ public Map> createOuterMap() { @Override public void putIfAbsent(final Map innerMap, final String name, final T logger) { - ((ConcurrentMap) innerMap).putIfAbsent(name, logger); + innerMap.putIfAbsent(name, logger); } } /** - * Generates WeakHashMaps for use by the registry to store the Loggers. + * {@link MapFactory} implementation using {@link WeakHashMap}. + * * @param subtype of {@code ExtendedLogger} */ public static class WeakMapFactory implements MapFactory { + @Override public Map createInnerMap() { return new WeakHashMap<>(); @@ -87,43 +106,63 @@ public void putIfAbsent(final Map innerMap, final String name, final } } - public LoggerRegistry() { - this(new ConcurrentMapFactory()); - } - - public LoggerRegistry(final MapFactory factory) { - this.factory = Objects.requireNonNull(factory, "factory"); - this.map = factory.createOuterMap(); - } + public LoggerRegistry() {} - private static String factoryClassKey(final Class messageFactoryClass) { - return messageFactoryClass == null ? DEFAULT_FACTORY_KEY : messageFactoryClass.getName(); - } - - private static String factoryKey(final MessageFactory messageFactory) { - return messageFactory == null - ? DEFAULT_FACTORY_KEY - : messageFactory.getClass().getName(); + /** + * Constructs an instance ignoring the given the map factory. + * + * @param mapFactory a map factory + */ + public LoggerRegistry(final MapFactory mapFactory) { + this(); } /** - * Returns an ExtendedLogger. - * @param name The name of the Logger to return. - * @return The logger with the specified name. + * Returns the logger associated with the given name. + *

+ * There can be made no assumptions on the message factory of the returned logger. + * Callers are strongly advised to switch to {@link #getLogger(String, MessageFactory)} and provide a message factory parameter! + *

+ * + * @param name a logger name + * @return the logger associated with the name */ public T getLogger(final String name) { - return getOrCreateInnerMap(DEFAULT_FACTORY_KEY).get(name); + requireNonNull(name, "name"); + return getLogger(name, null); } /** - * Returns an ExtendedLogger. - * @param name The name of the Logger to return. - * @param messageFactory The message factory is used only when creating a logger, subsequent use does not change - * the logger but will log a warning if mismatched. - * @return The logger with the specified name. + * Returns the logger associated with the given name and message factory. + *

+ * In the absence of a message factory, there can be made no assumptions on the message factory of the returned logger. + * This lenient behaviour is only kept for backward compatibility. + * Callers are strongly advised to provide a message factory parameter to the method! + *

+ * + * @param name a logger name + * @param messageFactory a message factory + * @return the logger associated with the given name and message factory */ public T getLogger(final String name, final MessageFactory messageFactory) { - return getOrCreateInnerMap(factoryKey(messageFactory)).get(name); + requireNonNull(name, "name"); + readLock.lock(); + try { + final Map> loggerRefByMessageFactory = + loggerRefByMessageFactoryByName.get(name); + if (loggerRefByMessageFactory == null) { + return null; + } + final MessageFactory effectiveMessageFactory = + messageFactory != null ? messageFactory : ParameterizedMessageFactory.INSTANCE; + final WeakReference loggerRef = loggerRefByMessageFactory.get(effectiveMessageFactory); + if (loggerRef == null) { + return null; + } + return loggerRef.get(); + } finally { + readLock.unlock(); + } } public Collection getLoggers() { @@ -131,53 +170,104 @@ public Collection getLoggers() { } public Collection getLoggers(final Collection destination) { - for (final Map inner : map.values()) { - destination.addAll(inner.values()); + requireNonNull(destination, "destination"); + readLock.lock(); + try { + loggerRefByMessageFactoryByName.values().stream() + .flatMap(loggerRefByMessageFactory -> + loggerRefByMessageFactory.values().stream().map(WeakReference::get)) + .filter(Objects::nonNull) + .forEach(destination::add); + } finally { + readLock.unlock(); } return destination; } - private Map getOrCreateInnerMap(final String factoryName) { - Map inner = map.get(factoryName); - if (inner == null) { - inner = factory.createInnerMap(); - map.put(factoryName, inner); - } - return inner; - } - /** - * Detects if a Logger with the specified name exists. - * @param name The Logger name to search for. - * @return true if the Logger exists, false otherwise. + * Checks if a logger associated with the given name exists. + *

+ * There can be made no assumptions on the message factory of the found logger. + * Callers are strongly advised to switch to {@link #hasLogger(String, MessageFactory)} and provide a message factory parameter! + *

+ * + * @param name a logger name + * @return {@code true}, if the logger exists; {@code false} otherwise. */ public boolean hasLogger(final String name) { - return getOrCreateInnerMap(DEFAULT_FACTORY_KEY).containsKey(name); + requireNonNull(name, "name"); + final T logger = getLogger(name); + return logger != null; } /** - * Detects if a Logger with the specified name and MessageFactory exists. - * @param name The Logger name to search for. - * @param messageFactory The message factory to search for. - * @return true if the Logger exists, false otherwise. + * Checks if a logger associated with the given name and message factory exists. + *

+ * In the absence of a message factory, there can be made no assumptions on the message factory of the found logger. + * This lenient behaviour is only kept for backward compatibility. + * Callers are strongly advised to provide a message factory parameter to the method! + *

+ * + * @param name a logger name + * @param messageFactory a message factory + * @return {@code true}, if the logger exists; {@code false} otherwise. * @since 2.5 */ public boolean hasLogger(final String name, final MessageFactory messageFactory) { - return getOrCreateInnerMap(factoryKey(messageFactory)).containsKey(name); + requireNonNull(name, "name"); + final T logger = getLogger(name, messageFactory); + return logger != null; } /** - * Detects if a Logger with the specified name and MessageFactory type exists. - * @param name The Logger name to search for. - * @param messageFactoryClass The message factory class to search for. - * @return true if the Logger exists, false otherwise. + * Checks if a logger associated with the given name and message factory type exists. + * + * @param name a logger name + * @param messageFactoryClass a message factory class + * @return {@code true}, if the logger exists; {@code false} otherwise. * @since 2.5 */ public boolean hasLogger(final String name, final Class messageFactoryClass) { - return getOrCreateInnerMap(factoryClassKey(messageFactoryClass)).containsKey(name); + requireNonNull(name, "name"); + requireNonNull(messageFactoryClass, "messageFactoryClass"); + readLock.lock(); + try { + return loggerRefByMessageFactoryByName.getOrDefault(name, Collections.emptyMap()).keySet().stream() + .anyMatch(messageFactory -> messageFactoryClass.equals(messageFactory.getClass())); + } finally { + readLock.unlock(); + } } + /** + * Registers the provided logger using the given name – message factory parameter is ignored and the one from the logger will be used instead. + * + * @param name ignored – kept for backward compatibility + * @param messageFactory ignored – kept for backward compatibility + * @param logger a logger instance + */ public void putIfAbsent(final String name, final MessageFactory messageFactory, final T logger) { - factory.putIfAbsent(getOrCreateInnerMap(factoryKey(messageFactory)), name, logger); + + // Check arguments + requireNonNull(logger, "logger"); + + // Insert the logger + writeLock.lock(); + try { + final Map> loggerRefByMessageFactory = + loggerRefByMessageFactoryByName.computeIfAbsent( + logger.getName(), this::createLoggerRefByMessageFactoryMap); + final MessageFactory loggerMessageFactory = logger.getMessageFactory(); + final WeakReference loggerRef = loggerRefByMessageFactory.get(loggerMessageFactory); + if (loggerRef == null || loggerRef.get() == null) { + loggerRefByMessageFactory.put(loggerMessageFactory, new WeakReference<>(logger)); + } + } finally { + writeLock.unlock(); + } + } + + private Map> createLoggerRefByMessageFactoryMap(final String ignored) { + return new WeakHashMap<>(); } } diff --git a/log4j-api/src/main/java/org/apache/logging/log4j/spi/package-info.java b/log4j-api/src/main/java/org/apache/logging/log4j/spi/package-info.java index 8fff7b80978..1748932083d 100644 --- a/log4j-api/src/main/java/org/apache/logging/log4j/spi/package-info.java +++ b/log4j-api/src/main/java/org/apache/logging/log4j/spi/package-info.java @@ -19,7 +19,7 @@ * API classes. */ @Export -@Version("2.24.0") +@Version("2.24.1") package org.apache.logging.log4j.spi; import org.osgi.annotation.bundle.Export; diff --git a/log4j-core-test/src/test/java/org/apache/logging/log4j/core/LoggerContextTest.java b/log4j-core-test/src/test/java/org/apache/logging/log4j/core/LoggerContextTest.java new file mode 100644 index 00000000000..0538b632d10 --- /dev/null +++ b/log4j-core-test/src/test/java/org/apache/logging/log4j/core/LoggerContextTest.java @@ -0,0 +1,40 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to you under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.logging.log4j.core; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.Mockito.mock; + +import org.apache.logging.log4j.message.MessageFactory; +import org.apache.logging.log4j.message.MessageFactory2; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.TestInfo; + +class LoggerContextTest { + + @Test + void newInstance_should_honor_name_and_message_factory(final TestInfo testInfo) { + final String testName = testInfo.getDisplayName(); + try (final LoggerContext loggerContext = new LoggerContext(testName)) { + final String loggerName = testName + "-loggerName"; + final MessageFactory2 messageFactory = mock(MessageFactory2.class); + final Logger logger = loggerContext.newInstance(loggerContext, loggerName, messageFactory); + assertThat(logger.getName()).isEqualTo(loggerName); + assertThat((MessageFactory) logger.getMessageFactory()).isSameAs(messageFactory); + } + } +} diff --git a/log4j-core-test/src/test/java/org/apache/logging/log4j/core/async/AsyncLoggerContextTest.java b/log4j-core-test/src/test/java/org/apache/logging/log4j/core/async/AsyncLoggerContextTest.java index 54403bbd9e2..82ee87d0123 100644 --- a/log4j-core-test/src/test/java/org/apache/logging/log4j/core/async/AsyncLoggerContextTest.java +++ b/log4j-core-test/src/test/java/org/apache/logging/log4j/core/async/AsyncLoggerContextTest.java @@ -16,23 +16,30 @@ */ package org.apache.logging.log4j.core.async; -import static org.junit.Assert.assertTrue; +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.Mockito.mock; import org.apache.logging.log4j.Logger; -import org.apache.logging.log4j.core.LoggerContext; -import org.apache.logging.log4j.core.test.CoreLoggerContexts; import org.apache.logging.log4j.core.test.categories.AsyncLoggers; -import org.junit.Test; +import org.apache.logging.log4j.message.MessageFactory; +import org.apache.logging.log4j.message.MessageFactory2; import org.junit.experimental.categories.Category; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.TestInfo; @Category(AsyncLoggers.class) -public class AsyncLoggerContextTest { +class AsyncLoggerContextTest { @Test - public void testNewInstanceReturnsAsyncLogger() { - final Logger logger = new AsyncLoggerContext("a").newInstance(new LoggerContext("a"), "a", null); - assertTrue(logger instanceof AsyncLogger); - - CoreLoggerContexts.stopLoggerContext(); // stop async thread + void verify_newInstance(final TestInfo testInfo) { + final String testName = testInfo.getDisplayName(); + try (final AsyncLoggerContext loggerContext = new AsyncLoggerContext(testName)) { + final String loggerName = testName + "-loggerName"; + final MessageFactory2 messageFactory = mock(MessageFactory2.class); + final Logger logger = loggerContext.newInstance(loggerContext, loggerName, messageFactory); + assertThat(logger).isInstanceOf(AsyncLogger.class); + assertThat(logger.getName()).isEqualTo(loggerName); + assertThat((MessageFactory) logger.getMessageFactory()).isSameAs(messageFactory); + } } } diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/Logger.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/Logger.java index 777d627115d..169c39162db 100644 --- a/log4j-core/src/main/java/org/apache/logging/log4j/core/Logger.java +++ b/log4j-core/src/main/java/org/apache/logging/log4j/core/Logger.java @@ -111,7 +111,7 @@ protected Logger( this.privateConfig = new PrivateConfig(context.getConfiguration(), this); } - private static MessageFactory getEffectiveMessageFactory(final MessageFactory messageFactory) { + static MessageFactory getEffectiveMessageFactory(final MessageFactory messageFactory) { return createInstanceFromFactoryProperty( MessageFactory.class, messageFactory, diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/LoggerContext.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/LoggerContext.java index 07cd2d1f592..ee8e7c6a3cd 100644 --- a/log4j-core/src/main/java/org/apache/logging/log4j/core/LoggerContext.java +++ b/log4j-core/src/main/java/org/apache/logging/log4j/core/LoggerContext.java @@ -48,7 +48,6 @@ import org.apache.logging.log4j.core.util.NetUtils; import org.apache.logging.log4j.core.util.ShutdownCallbackRegistry; import org.apache.logging.log4j.message.MessageFactory; -import org.apache.logging.log4j.spi.AbstractLogger; import org.apache.logging.log4j.spi.LoggerContextFactory; import org.apache.logging.log4j.spi.LoggerContextShutdownAware; import org.apache.logging.log4j.spi.LoggerContextShutdownEnabled; @@ -76,6 +75,13 @@ public class LoggerContext extends AbstractLifeCycle private static final Configuration NULL_CONFIGURATION = new NullConfiguration(); + /** + * The default message factory to use while creating loggers, if none is provided. + * + * @see #2936 for the discussion on why we leak the message factory of the default logger and hardcode it here. + */ + private static final MessageFactory DEFAULT_MESSAGE_FACTORY = Logger.getEffectiveMessageFactory(null); + private final LoggerRegistry loggerRegistry = new LoggerRegistry<>(); private final CopyOnWriteArrayList propertyChangeListeners = new CopyOnWriteArrayList<>(); private volatile List listeners; @@ -498,7 +504,7 @@ public Object getExternalContext() { */ @Override public Logger getLogger(final String name) { - return getLogger(name, null); + return getLogger(name, DEFAULT_MESSAGE_FACTORY); } /** @@ -515,25 +521,23 @@ public Collection getLoggers() { } /** - * Obtains a Logger from the Context. + * Obtains a logger from the context. * - * @param name The name of the Logger to return. - * @param messageFactory The message factory is used only when creating a logger, subsequent use does not change the - * logger but will log a warning if mismatched. - * @return The Logger. + * @param name a logger name + * @param messageFactory a message factory to associate the logger with + * @return a logger matching the given name and message factory */ @Override public Logger getLogger(final String name, final MessageFactory messageFactory) { - // Note: This is the only method where we add entries to the 'loggerRegistry' ivar. - Logger logger = loggerRegistry.getLogger(name, messageFactory); - if (logger != null) { - AbstractLogger.checkMessageFactory(logger, messageFactory); - return logger; + final MessageFactory effectiveMessageFactory = + messageFactory != null ? messageFactory : DEFAULT_MESSAGE_FACTORY; + final Logger oldLogger = loggerRegistry.getLogger(name, effectiveMessageFactory); + if (oldLogger != null) { + return oldLogger; } - - logger = newInstance(this, name, messageFactory); - loggerRegistry.putIfAbsent(name, messageFactory, logger); - return loggerRegistry.getLogger(name, messageFactory); + final Logger newLogger = newInstance(name, effectiveMessageFactory); + loggerRegistry.putIfAbsent(name, effectiveMessageFactory, newLogger); + return loggerRegistry.getLogger(name, effectiveMessageFactory); } /** @@ -554,7 +558,7 @@ public LoggerRegistry getLoggerRegistry() { */ @Override public boolean hasLogger(final String name) { - return loggerRegistry.hasLogger(name); + return loggerRegistry.hasLogger(name, DEFAULT_MESSAGE_FACTORY); } /** @@ -565,7 +569,9 @@ public boolean hasLogger(final String name) { */ @Override public boolean hasLogger(final String name, final MessageFactory messageFactory) { - return loggerRegistry.hasLogger(name, messageFactory); + final MessageFactory effectiveMessageFactory = + messageFactory != null ? messageFactory : DEFAULT_MESSAGE_FACTORY; + return loggerRegistry.hasLogger(name, effectiveMessageFactory); } /** @@ -812,6 +818,10 @@ private void initApiModule() { .init(); // Or make public and call ThreadContext.init() which calls ThreadContextMapFactory.init(). } + private Logger newInstance(final String name, final MessageFactory messageFactory) { + return newInstance(this, name, messageFactory); + } + // LOG4J2-151: changed visibility from private to protected protected Logger newInstance(final LoggerContext ctx, final String name, final MessageFactory messageFactory) { return new Logger(ctx, name, messageFactory); diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/package-info.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/package-info.java index 266256b4637..7c02b12fe60 100644 --- a/log4j-core/src/main/java/org/apache/logging/log4j/core/package-info.java +++ b/log4j-core/src/main/java/org/apache/logging/log4j/core/package-info.java @@ -18,7 +18,7 @@ * Implementation of Log4j 2. */ @Export -@Version("2.24.0") +@Version("2.24.1") package org.apache.logging.log4j.core; import org.osgi.annotation.bundle.Export; diff --git a/log4j-taglib/src/main/java/org/apache/logging/log4j/taglib/Log4jTaglibLoggerContext.java b/log4j-taglib/src/main/java/org/apache/logging/log4j/taglib/Log4jTaglibLoggerContext.java index 3c76f39b21a..30f1a1f0299 100644 --- a/log4j-taglib/src/main/java/org/apache/logging/log4j/taglib/Log4jTaglibLoggerContext.java +++ b/log4j-taglib/src/main/java/org/apache/logging/log4j/taglib/Log4jTaglibLoggerContext.java @@ -16,11 +16,15 @@ */ package org.apache.logging.log4j.taglib; +import java.util.Map; import java.util.WeakHashMap; +import java.util.concurrent.locks.Lock; +import java.util.concurrent.locks.ReadWriteLock; +import java.util.concurrent.locks.ReentrantReadWriteLock; import javax.servlet.ServletContext; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.message.MessageFactory; -import org.apache.logging.log4j.spi.AbstractLogger; +import org.apache.logging.log4j.message.ParameterizedMessageFactory; import org.apache.logging.log4j.spi.ExtendedLogger; import org.apache.logging.log4j.spi.LoggerContext; import org.apache.logging.log4j.spi.LoggerRegistry; @@ -32,12 +36,19 @@ * @since 2.0 */ final class Log4jTaglibLoggerContext implements LoggerContext { - // These were change to WeakHashMaps to avoid ClassLoader (memory) leak, something that's particularly - // important in Servlet containers. - private static final WeakHashMap CONTEXTS = new WeakHashMap<>(); - private final LoggerRegistry loggerRegistry = - new LoggerRegistry<>(new LoggerRegistry.WeakMapFactory()); + private static final ReadWriteLock LOCK = new ReentrantReadWriteLock(); + + private static final Lock READ_LOCK = LOCK.readLock(); + + private static final Lock WRITE_LOCK = LOCK.writeLock(); + + private static final Map LOGGER_CONTEXT_BY_SERVLET_CONTEXT = + new WeakHashMap<>(); + + private static final MessageFactory DEFAULT_MESSAGE_FACTORY = ParameterizedMessageFactory.INSTANCE; + + private final LoggerRegistry loggerRegistry = new LoggerRegistry<>(); private final ServletContext servletContext; @@ -47,36 +58,31 @@ private Log4jTaglibLoggerContext(final ServletContext servletContext) { @Override public Object getExternalContext() { - return this.servletContext; + return servletContext; } @Override public Log4jTaglibLogger getLogger(final String name) { - return this.getLogger(name, null); + return getLogger(name, null); } @Override public Log4jTaglibLogger getLogger(final String name, final MessageFactory messageFactory) { - // Note: This is the only method where we add entries to the 'loggerRegistry' ivar. - Log4jTaglibLogger logger = this.loggerRegistry.getLogger(name, messageFactory); - if (logger != null) { - AbstractLogger.checkMessageFactory(logger, messageFactory); - return logger; - } - - synchronized (this.loggerRegistry) { - logger = this.loggerRegistry.getLogger(name, messageFactory); - if (logger == null) { - final LoggerContext context = LogManager.getContext(false); - final ExtendedLogger original = - messageFactory == null ? context.getLogger(name) : context.getLogger(name, messageFactory); - // wrap a logger from an underlying implementation - logger = new Log4jTaglibLogger(original, name, original.getMessageFactory()); - this.loggerRegistry.putIfAbsent(name, messageFactory, logger); - } + final MessageFactory effectiveMessageFactory = + messageFactory != null ? messageFactory : DEFAULT_MESSAGE_FACTORY; + final Log4jTaglibLogger oldLogger = loggerRegistry.getLogger(name, effectiveMessageFactory); + if (oldLogger != null) { + return oldLogger; } + final Log4jTaglibLogger newLogger = createLogger(name, effectiveMessageFactory); + loggerRegistry.putIfAbsent(name, effectiveMessageFactory, newLogger); + return loggerRegistry.getLogger(name, effectiveMessageFactory); + } - return logger; + private Log4jTaglibLogger createLogger(final String name, final MessageFactory messageFactory) { + final LoggerContext loggerContext = LogManager.getContext(false); + final ExtendedLogger delegateLogger = loggerContext.getLogger(name, messageFactory); + return new Log4jTaglibLogger(delegateLogger, name, delegateLogger.getMessageFactory()); } @Override @@ -86,7 +92,9 @@ public boolean hasLogger(final String name) { @Override public boolean hasLogger(final String name, final MessageFactory messageFactory) { - return loggerRegistry.hasLogger(name, messageFactory); + final MessageFactory effectiveMessageFactory = + messageFactory != null ? messageFactory : DEFAULT_MESSAGE_FACTORY; + return loggerRegistry.hasLogger(name, effectiveMessageFactory); } @Override @@ -94,20 +102,25 @@ public boolean hasLogger(final String name, final ClassMichael Vorburger.ch for Google */ class JULLoggerContext implements LoggerContext { + private final LoggerRegistry loggerRegistry = new LoggerRegistry<>(); + private static final MessageFactory DEFAULT_MESSAGE_FACTORY = ParameterizedMessageFactory.INSTANCE; + // This implementation is strongly inspired by org.apache.logging.slf4j.SLF4JLoggerContext @Override @@ -40,29 +44,37 @@ public Object getExternalContext() { @Override public ExtendedLogger getLogger(final String name) { - if (!loggerRegistry.hasLogger(name)) { - loggerRegistry.putIfAbsent(name, null, new JULLogger(name, Logger.getLogger(name))); - } - return loggerRegistry.getLogger(name); + return getLogger(name, null); } @Override public ExtendedLogger getLogger(final String name, final MessageFactory messageFactory) { - if (!loggerRegistry.hasLogger(name, messageFactory)) { - loggerRegistry.putIfAbsent( - name, messageFactory, new JULLogger(name, messageFactory, Logger.getLogger(name))); + final MessageFactory effectiveMessageFactory = + messageFactory != null ? messageFactory : DEFAULT_MESSAGE_FACTORY; + final ExtendedLogger oldLogger = loggerRegistry.getLogger(name, effectiveMessageFactory); + if (oldLogger != null) { + return oldLogger; } - return loggerRegistry.getLogger(name, messageFactory); + final ExtendedLogger newLogger = createLogger(name, effectiveMessageFactory); + loggerRegistry.putIfAbsent(name, effectiveMessageFactory, newLogger); + return loggerRegistry.getLogger(name, effectiveMessageFactory); + } + + private static ExtendedLogger createLogger(final String name, final MessageFactory messageFactory) { + final Logger logger = Logger.getLogger(name); + return new JULLogger(name, messageFactory, logger); } @Override public boolean hasLogger(final String name) { - return loggerRegistry.hasLogger(name); + return loggerRegistry.hasLogger(name, DEFAULT_MESSAGE_FACTORY); } @Override public boolean hasLogger(final String name, final MessageFactory messageFactory) { - return loggerRegistry.hasLogger(name, messageFactory); + final MessageFactory effectiveMessageFactory = + messageFactory != null ? messageFactory : DEFAULT_MESSAGE_FACTORY; + return loggerRegistry.hasLogger(name, effectiveMessageFactory); } @Override diff --git a/log4j-to-jul/src/main/java/org/apache/logging/log4j/tojul/package-info.java b/log4j-to-jul/src/main/java/org/apache/logging/log4j/tojul/package-info.java index aa117c7995c..8308d5c2faf 100644 --- a/log4j-to-jul/src/main/java/org/apache/logging/log4j/tojul/package-info.java +++ b/log4j-to-jul/src/main/java/org/apache/logging/log4j/tojul/package-info.java @@ -21,7 +21,7 @@ * @author Michael Vorburger.ch for Google */ @Export -@Version("2.24.0") +@Version("2.24.1") package org.apache.logging.log4j.tojul; import org.osgi.annotation.bundle.Export; diff --git a/log4j-to-slf4j/src/main/java/org/apache/logging/slf4j/SLF4JLoggerContext.java b/log4j-to-slf4j/src/main/java/org/apache/logging/slf4j/SLF4JLoggerContext.java index f0aee0af414..e16a9a177ca 100644 --- a/log4j-to-slf4j/src/main/java/org/apache/logging/slf4j/SLF4JLoggerContext.java +++ b/log4j-to-slf4j/src/main/java/org/apache/logging/slf4j/SLF4JLoggerContext.java @@ -17,14 +17,19 @@ package org.apache.logging.slf4j; import org.apache.logging.log4j.message.MessageFactory; +import org.apache.logging.log4j.message.ParameterizedMessageFactory; import org.apache.logging.log4j.spi.ExtendedLogger; import org.apache.logging.log4j.spi.LoggerContext; import org.apache.logging.log4j.spi.LoggerRegistry; +import org.slf4j.Logger; import org.slf4j.LoggerFactory; public class SLF4JLoggerContext implements LoggerContext { + private final LoggerRegistry loggerRegistry = new LoggerRegistry<>(); + private static final MessageFactory DEFAULT_MESSAGE_FACTORY = ParameterizedMessageFactory.INSTANCE; + @Override public Object getExternalContext() { return null; @@ -32,29 +37,37 @@ public Object getExternalContext() { @Override public ExtendedLogger getLogger(final String name) { - if (!loggerRegistry.hasLogger(name)) { - loggerRegistry.putIfAbsent(name, null, new SLF4JLogger(name, LoggerFactory.getLogger(name))); - } - return loggerRegistry.getLogger(name); + return getLogger(name, null); } @Override public ExtendedLogger getLogger(final String name, final MessageFactory messageFactory) { - if (!loggerRegistry.hasLogger(name, messageFactory)) { - loggerRegistry.putIfAbsent( - name, messageFactory, new SLF4JLogger(name, messageFactory, LoggerFactory.getLogger(name))); + final MessageFactory effectiveMessageFactory = + messageFactory != null ? messageFactory : DEFAULT_MESSAGE_FACTORY; + final ExtendedLogger oldLogger = loggerRegistry.getLogger(name, effectiveMessageFactory); + if (oldLogger != null) { + return oldLogger; } - return loggerRegistry.getLogger(name, messageFactory); + final ExtendedLogger newLogger = createLogger(name, effectiveMessageFactory); + loggerRegistry.putIfAbsent(name, effectiveMessageFactory, newLogger); + return loggerRegistry.getLogger(name, effectiveMessageFactory); + } + + private static ExtendedLogger createLogger(final String name, final MessageFactory messageFactory) { + final Logger logger = LoggerFactory.getLogger(name); + return new SLF4JLogger(name, messageFactory, logger); } @Override public boolean hasLogger(final String name) { - return loggerRegistry.hasLogger(name); + return loggerRegistry.hasLogger(name, DEFAULT_MESSAGE_FACTORY); } @Override public boolean hasLogger(final String name, final MessageFactory messageFactory) { - return loggerRegistry.hasLogger(name, messageFactory); + final MessageFactory effectiveMessageFactory = + messageFactory != null ? messageFactory : DEFAULT_MESSAGE_FACTORY; + return loggerRegistry.hasLogger(name, effectiveMessageFactory); } @Override diff --git a/log4j-to-slf4j/src/main/java/org/apache/logging/slf4j/package-info.java b/log4j-to-slf4j/src/main/java/org/apache/logging/slf4j/package-info.java index ba4cb130be1..12e4bedba29 100644 --- a/log4j-to-slf4j/src/main/java/org/apache/logging/slf4j/package-info.java +++ b/log4j-to-slf4j/src/main/java/org/apache/logging/slf4j/package-info.java @@ -18,7 +18,7 @@ * SLF4J support. */ @Export -@Version("2.24.0") +@Version("2.24.1") package org.apache.logging.slf4j; import org.osgi.annotation.bundle.Export; diff --git a/src/changelog/.2.x.x/2936_make_LoggerRegistry_MessageFactory_namespaced.xml b/src/changelog/.2.x.x/2936_make_LoggerRegistry_MessageFactory_namespaced.xml new file mode 100644 index 00000000000..c314f84251d --- /dev/null +++ b/src/changelog/.2.x.x/2936_make_LoggerRegistry_MessageFactory_namespaced.xml @@ -0,0 +1,9 @@ + + + + Rework `LoggerRegistry` to make it `MessageFactory`-namespaced. +This effectively allows loggers of same name, but different message factory. +