diff --git a/log4j-api-test/src/test/java/org/apache/logging/log4j/spi/LoggerRegistryTest.java b/log4j-api-test/src/test/java/org/apache/logging/log4j/spi/LoggerRegistryTest.java new file mode 100644 index 00000000000..269785ea714 --- /dev/null +++ b/log4j-api-test/src/test/java/org/apache/logging/log4j/spi/LoggerRegistryTest.java @@ -0,0 +1,64 @@ +/* + * 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.spi; + +import static org.assertj.core.api.Assertions.assertThat; + +import java.lang.ref.WeakReference; +import java.util.stream.Stream; +import org.apache.logging.log4j.message.MessageFactory; +import org.apache.logging.log4j.message.ParameterizedMessageFactory; +import org.apache.logging.log4j.message.ReusableMessageFactory; +import org.apache.logging.log4j.test.TestLogger; +import org.jspecify.annotations.Nullable; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.MethodSource; + +class LoggerRegistryTest { + + private static final String LOGGER_NAME = LoggerRegistryTest.class.getName(); + + static Stream<@Nullable MessageFactory> doesNotLoseLoggerReferences() { + return Stream.of( + ParameterizedMessageFactory.INSTANCE, + ReusableMessageFactory.INSTANCE, + new ParameterizedMessageFactory(), + null); + } + + /** + * @see > loggerRefByMessageFactory = - loggerRefByMessageFactoryByName.get(name); - if (loggerRefByMessageFactory == null) { - return null; - } + final @Nullable Map loggerByMessageFactory = loggerByMessageFactoryByName.get(name); final MessageFactory effectiveMessageFactory = messageFactory != null ? messageFactory : ParameterizedMessageFactory.INSTANCE; - final WeakReference loggerRef = loggerRefByMessageFactory.get(effectiveMessageFactory); - if (loggerRef == null) { - return null; - } - return loggerRef.get(); + return loggerByMessageFactory == null ? null : loggerByMessageFactory.get(effectiveMessageFactory); } finally { readLock.unlock(); } } public Collection getLoggers() { - return getLoggers(new ArrayList()); + return getLoggers(new ArrayList<>()); } public Collection getLoggers(final Collection destination) { requireNonNull(destination, "destination"); readLock.lock(); try { - loggerRefByMessageFactoryByName.values().stream() - .flatMap(loggerRefByMessageFactory -> - loggerRefByMessageFactory.values().stream().map(WeakReference::get)) - .filter(Objects::nonNull) + loggerByMessageFactoryByName.values().stream() + .flatMap(loggerByMessageFactory -> loggerByMessageFactory.values().stream()) .forEach(destination::add); } finally { readLock.unlock(); @@ -215,7 +201,7 @@ public Collection getLoggers(final Collection destination) { @Deprecated public boolean hasLogger(final String name) { requireNonNull(name, "name"); - final T logger = getLogger(name); + final @Nullable T logger = getLogger(name); return logger != null; } @@ -234,7 +220,7 @@ public boolean hasLogger(final String name) { */ public boolean hasLogger(final String name, @Nullable final MessageFactory messageFactory) { requireNonNull(name, "name"); - final T logger = getLogger(name, messageFactory); + final @Nullable T logger = getLogger(name, messageFactory); return logger != null; } @@ -251,7 +237,7 @@ public boolean hasLogger(final String name, final Class messageFactoryClass.equals(messageFactory.getClass())); } finally { readLock.unlock(); @@ -262,91 +248,30 @@ public boolean hasLogger(final String name, final ClassLogger name and message factory parameters are ignored, those will be obtained from the logger instead. * - * @param name ignored – kept for backward compatibility - * @param messageFactory ignored – kept for backward compatibility + * @param name a logger name + * @param messageFactory a message factory * @param logger a logger instance - * @deprecated As of version {@code 2.25.0}, planned to be removed! - * Use {@link #computeIfAbsent(String, MessageFactory, BiFunction)} instead. */ - @Deprecated - public void putIfAbsent( - @Nullable final String name, @Nullable final MessageFactory messageFactory, final T logger) { + public void putIfAbsent(final String name, @Nullable final MessageFactory messageFactory, final T logger) { // Check arguments + requireNonNull(name, "name"); requireNonNull(logger, "logger"); // Insert the logger writeLock.lock(); try { - final String loggerName = logger.getName(); - final Map> loggerRefByMessageFactory = - loggerRefByMessageFactoryByName.computeIfAbsent( - loggerName, 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(); - } - } - - public T computeIfAbsent( - final String name, - final MessageFactory messageFactory, - final BiFunction loggerSupplier) { - - // Check arguments - requireNonNull(name, "name"); - requireNonNull(messageFactory, "messageFactory"); - requireNonNull(loggerSupplier, "loggerSupplier"); - - // Read lock fast path: See if logger already exists - T logger = getLogger(name, messageFactory); - if (logger != null) { - return logger; - } - - // Write lock slow path: Insert the logger - writeLock.lock(); - try { - - // See if the logger is created by another thread in the meantime - final Map> loggerRefByMessageFactory = - loggerRefByMessageFactoryByName.computeIfAbsent(name, this::createLoggerRefByMessageFactoryMap); - final WeakReference loggerRef; - if ((loggerRef = loggerRefByMessageFactory.get(messageFactory)) != null - && (logger = loggerRef.get()) != null) { - return logger; - } - - // Create the logger - logger = loggerSupplier.apply(name, messageFactory); - - // Report message factory mismatches, if there is any - final MessageFactory loggerMessageFactory = logger.getMessageFactory(); - if (!loggerMessageFactory.equals(messageFactory)) { - StatusLogger.getLogger() - .error( - "Newly registered logger with name `{}` and message factory `{}`, is requested to be associated with a different message factory: `{}`.\n" - + "Effectively the message factory of the logger will be used and the other one will be ignored.\n" - + "This generally hints a problem at the logger context implementation.\n" - + "Please report this using the Log4j project issue tracker.", - name, - loggerMessageFactory, - messageFactory); - } - - // Insert the logger - loggerRefByMessageFactory.put(loggerMessageFactory, new WeakReference<>(logger)); - return logger; + final MessageFactory effectiveMessageFactory = + messageFactory != null ? messageFactory : ParameterizedMessageFactory.INSTANCE; + loggerByMessageFactoryByName + .computeIfAbsent(name, this::createLoggerRefByMessageFactoryMap) + .putIfAbsent(effectiveMessageFactory, logger); } finally { writeLock.unlock(); } } - private Map> createLoggerRefByMessageFactoryMap(final String ignored) { + 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 3b6b5c25857..1637d7a3d16 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.25.0") +@Version("2.24.2") package org.apache.logging.log4j.spi; import org.osgi.annotation.bundle.Export; 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 df8bb774819..0eac222c9e2 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 @@ -33,6 +33,7 @@ import java.util.concurrent.TimeUnit; import java.util.concurrent.locks.Lock; import java.util.concurrent.locks.ReentrantLock; +import java.util.stream.Collectors; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.core.config.Configuration; import org.apache.logging.log4j.core.config.ConfigurationFactory; @@ -47,11 +48,11 @@ import org.apache.logging.log4j.core.util.ExecutorServices; import org.apache.logging.log4j.core.util.NetUtils; import org.apache.logging.log4j.core.util.ShutdownCallbackRegistry; +import org.apache.logging.log4j.core.util.internal.InternalLoggerRegistry; import org.apache.logging.log4j.message.MessageFactory; import org.apache.logging.log4j.spi.LoggerContextFactory; import org.apache.logging.log4j.spi.LoggerContextShutdownAware; import org.apache.logging.log4j.spi.LoggerContextShutdownEnabled; -import org.apache.logging.log4j.spi.LoggerRegistry; import org.apache.logging.log4j.spi.Terminable; import org.apache.logging.log4j.spi.ThreadContextMapFactory; import org.apache.logging.log4j.util.PropertiesUtil; @@ -83,7 +84,7 @@ public class LoggerContext extends AbstractLifeCycle */ private static final MessageFactory DEFAULT_MESSAGE_FACTORY = Logger.getEffectiveMessageFactory(null); - private final LoggerRegistry loggerRegistry = new LoggerRegistry<>(); + private final InternalLoggerRegistry loggerRegistry = new InternalLoggerRegistry(); private final CopyOnWriteArrayList propertyChangeListeners = new CopyOnWriteArrayList<>(); private volatile List listeners; @@ -513,7 +514,7 @@ public Logger getLogger(final String name) { * @return a collection of the current loggers. */ public Collection getLoggers() { - return loggerRegistry.getLoggers(); + return loggerRegistry.getLoggers().collect(Collectors.toList()); } /** @@ -535,9 +536,14 @@ public Logger getLogger(final String name, @Nullable final MessageFactory messag * * @return the LoggerRegistry. * @since 2.17.2 + * @deprecated since 2.25.0 without a replacement. */ - public LoggerRegistry getLoggerRegistry() { - return loggerRegistry; + @Deprecated + public org.apache.logging.log4j.spi.LoggerRegistry getLoggerRegistry() { + org.apache.logging.log4j.spi.LoggerRegistry result = + new org.apache.logging.log4j.spi.LoggerRegistry<>(); + loggerRegistry.getLoggers().forEach(l -> result.putIfAbsent(l.getName(), l.getMessageFactory(), l)); + return result; } /** @@ -770,9 +776,7 @@ public void updateLoggers() { */ public void updateLoggers(final Configuration config) { final Configuration old = this.configuration; - for (final Logger logger : loggerRegistry.getLoggers()) { - logger.updateConfiguration(config); - } + loggerRegistry.getLoggers().forEach(logger -> logger.updateConfiguration(config)); firePropertyChangeEvent(new PropertyChangeEvent(this, PROPERTY_CONFIG, old, config)); } diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/util/internal/InternalLoggerRegistry.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/util/internal/InternalLoggerRegistry.java new file mode 100644 index 00000000000..8900d540c1c --- /dev/null +++ b/log4j-core/src/main/java/org/apache/logging/log4j/core/util/internal/InternalLoggerRegistry.java @@ -0,0 +1,180 @@ +/* + * 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.util.internal; + +import static java.util.Objects.requireNonNull; + +import java.lang.ref.WeakReference; +import java.util.HashMap; +import java.util.Map; +import java.util.Optional; +import java.util.WeakHashMap; +import java.util.concurrent.locks.Lock; +import java.util.concurrent.locks.ReadWriteLock; +import java.util.concurrent.locks.ReentrantReadWriteLock; +import java.util.function.BiFunction; +import java.util.stream.Stream; +import org.apache.logging.log4j.core.Logger; +import org.apache.logging.log4j.message.MessageFactory; +import org.apache.logging.log4j.status.StatusLogger; +import org.jspecify.annotations.NullMarked; +import org.jspecify.annotations.Nullable; + +/** + * Convenience class used by {@link org.apache.logging.log4j.core.LoggerContext} + *

+ * We don't use {@link org.apache.logging.log4j.spi.LoggerRegistry} from the Log4j API to keep Log4j Core independent + * from the version of the Log4j API at runtime. + *

+ * @since 2.25.0 + */ +@NullMarked +public final class InternalLoggerRegistry { + + private final Map>> loggerRefByNameByMessageFactory = + new WeakHashMap<>(); + + private final ReadWriteLock lock = new ReentrantReadWriteLock(); + + private final Lock readLock = lock.readLock(); + + private final Lock writeLock = lock.writeLock(); + + public InternalLoggerRegistry() {} + + /** + * Returns the logger associated with the given name and message factory. + * + * @param name a logger name + * @param messageFactory a message factory + * @return the logger associated with the given name and message factory + */ + public @Nullable Logger getLogger(final String name, final MessageFactory messageFactory) { + requireNonNull(name, "name"); + requireNonNull(messageFactory, "messageFactory"); + readLock.lock(); + try { + return Optional.of(loggerRefByNameByMessageFactory) + .map(loggerRefByNameByMessageFactory -> loggerRefByNameByMessageFactory.get(messageFactory)) + .map(loggerRefByName -> loggerRefByName.get(name)) + .map(WeakReference::get) + .orElse(null); + } finally { + readLock.unlock(); + } + } + + public Stream getLoggers() { + readLock.lock(); + try { + return loggerRefByNameByMessageFactory.values().stream() + .flatMap(loggerRefByName -> loggerRefByName.values().stream()) + .flatMap(loggerRef -> { + @Nullable Logger logger = loggerRef.get(); + return logger != null ? Stream.of(logger) : Stream.empty(); + }); + } finally { + readLock.unlock(); + } + } + + /** + * Checks if a logger associated with the given name and message factory exists. + * + * @param name a logger name + * @param messageFactory a message factory + * @return {@code true}, if the logger exists; {@code false} otherwise. + */ + public boolean hasLogger(final String name, final MessageFactory messageFactory) { + requireNonNull(name, "name"); + requireNonNull(messageFactory, "messageFactory"); + return getLogger(name, messageFactory) != null; + } + + /** + * 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. + */ + public boolean hasLogger(final String name, final Class messageFactoryClass) { + requireNonNull(name, "name"); + requireNonNull(messageFactoryClass, "messageFactoryClass"); + readLock.lock(); + try { + return loggerRefByNameByMessageFactory.entrySet().stream() + .filter(entry -> messageFactoryClass.equals(entry.getKey().getClass())) + .anyMatch(entry -> entry.getValue().containsKey(name)); + } finally { + readLock.unlock(); + } + } + + public Logger computeIfAbsent( + final String name, + final MessageFactory messageFactory, + final BiFunction loggerSupplier) { + + // Check arguments + requireNonNull(name, "name"); + requireNonNull(messageFactory, "messageFactory"); + requireNonNull(loggerSupplier, "loggerSupplier"); + + // Read lock fast path: See if logger already exists + @Nullable Logger logger = getLogger(name, messageFactory); + if (logger != null) { + return logger; + } + + // Write lock slow path: Insert the logger + writeLock.lock(); + try { + + // See if the logger is created by another thread in the meantime + final Map> loggerRefByName = + loggerRefByNameByMessageFactory.computeIfAbsent(messageFactory, ignored -> new HashMap<>()); + WeakReference loggerRef = loggerRefByName.get(name); + if (loggerRef != null && (logger = loggerRef.get()) != null) { + return logger; + } + + // Create the logger + logger = loggerSupplier.apply(name, messageFactory); + + // Report message factory mismatches, if there is any + final MessageFactory loggerMessageFactory = logger.getMessageFactory(); + if (!loggerMessageFactory.equals(messageFactory)) { + StatusLogger.getLogger() + .error( + "Newly registered logger with name `{}` and message factory `{}`, is requested to be associated with a different message factory: `{}`.\n" + + "Effectively the message factory of the logger will be used and the other one will be ignored.\n" + + "This generally hints a problem at the logger context implementation.\n" + + "Please report this using the Log4j project issue tracker.", + name, + loggerMessageFactory, + messageFactory); + } + + // Insert the logger + loggerRefByName.put(name, new WeakReference<>(logger)); + return logger; + } finally { + writeLock.unlock(); + } + } +} 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 5d25ff6d89d..97a7ab28bbe 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 @@ -73,10 +73,16 @@ public Log4jTaglibLogger getLogger(final String name) { public Log4jTaglibLogger getLogger(final String name, @Nullable final MessageFactory messageFactory) { final MessageFactory effectiveMessageFactory = messageFactory != null ? messageFactory : DEFAULT_MESSAGE_FACTORY; - return loggerRegistry.computeIfAbsent(name, effectiveMessageFactory, this::createLogger); + 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); } - private Log4jTaglibLogger createLogger(final String name, @Nullable final MessageFactory messageFactory) { + 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()); diff --git a/log4j-to-jul/src/main/java/org/apache/logging/log4j/tojul/JULLoggerContext.java b/log4j-to-jul/src/main/java/org/apache/logging/log4j/tojul/JULLoggerContext.java index 9f92dca4931..6c18a83d485 100644 --- a/log4j-to-jul/src/main/java/org/apache/logging/log4j/tojul/JULLoggerContext.java +++ b/log4j-to-jul/src/main/java/org/apache/logging/log4j/tojul/JULLoggerContext.java @@ -52,7 +52,13 @@ public ExtendedLogger getLogger(final String name) { public ExtendedLogger getLogger(final String name, @Nullable final MessageFactory messageFactory) { final MessageFactory effectiveMessageFactory = messageFactory != null ? messageFactory : DEFAULT_MESSAGE_FACTORY; - return loggerRegistry.computeIfAbsent(name, effectiveMessageFactory, JULLoggerContext::createLogger); + final ExtendedLogger oldLogger = loggerRegistry.getLogger(name, effectiveMessageFactory); + if (oldLogger != null) { + return oldLogger; + } + final ExtendedLogger newLogger = createLogger(name, effectiveMessageFactory); + loggerRegistry.putIfAbsent(name, effectiveMessageFactory, newLogger); + return loggerRegistry.getLogger(name, effectiveMessageFactory); } private static ExtendedLogger createLogger(final String name, @Nullable final MessageFactory messageFactory) { 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 ba2a1d48189..50160e9063c 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 @@ -45,7 +45,13 @@ public ExtendedLogger getLogger(final String name) { public ExtendedLogger getLogger(final String name, @Nullable final MessageFactory messageFactory) { final MessageFactory effectiveMessageFactory = messageFactory != null ? messageFactory : DEFAULT_MESSAGE_FACTORY; - return loggerRegistry.computeIfAbsent(name, effectiveMessageFactory, SLF4JLoggerContext::createLogger); + final ExtendedLogger oldLogger = loggerRegistry.getLogger(name, effectiveMessageFactory); + if (oldLogger != null) { + return oldLogger; + } + final ExtendedLogger newLogger = createLogger(name, effectiveMessageFactory); + loggerRegistry.putIfAbsent(name, effectiveMessageFactory, newLogger); + return loggerRegistry.getLogger(name, effectiveMessageFactory); } private static ExtendedLogger createLogger(final String name, @Nullable final MessageFactory messageFactory) { diff --git a/src/changelog/.2.x.x/3143_logger_registry.xml b/src/changelog/.2.x.x/3143_logger_registry.xml new file mode 100644 index 00000000000..d184e022a6a --- /dev/null +++ b/src/changelog/.2.x.x/3143_logger_registry.xml @@ -0,0 +1,10 @@ + + + + + Use hard references to `Logger`s in `LoggerRegistry`. + +