From 462b0fc6ac9840d6c4c1ee0082089eca55ebd6b6 Mon Sep 17 00:00:00 2001 From: "Piotr P. Karwasz" Date: Fri, 15 Nov 2024 22:29:10 +0100 Subject: [PATCH] Removes weak references from `LoggerRepository` (#3199) Removes weak references to `Logger`s in `LoggerRepository`. The usage of weak references in `LoggerRepository` might cause `null` to be returned by `LogManager.getLogger()` of all Log4j Core versions up to `2.24.1`. Versions of Log4j API up to `2.24.0` did hold **hard** references to all the registered loggers, so the change will not alter the previous behavior. This PR also inverts the order of the `String` and `MessageFactory` keys to the `LoggerRepository` multi-map to limit the number of internal maps. The external map is a `WeakHashMap` to allow logger-specific message factories to be GC-ed. Closes #3143. --- log4j-api-test/pom.xml | 5 + .../logging/log4j/spi/LoggerRegistryTest.java | 64 +++++++ .../logging/log4j/spi/LoggerRegistry.java | 57 +++--- .../logging/log4j/spi/package-info.java | 2 +- .../logging/log4j/core/LoggerContext.java | 28 ++- .../util/internal/InternalLoggerRegistry.java | 180 ++++++++++++++++++ src/changelog/.2.x.x/3143_logger_registry.xml | 10 + 7 files changed, 295 insertions(+), 51 deletions(-) create mode 100644 log4j-api-test/src/test/java/org/apache/logging/log4j/spi/LoggerRegistryTest.java create mode 100644 log4j-core/src/main/java/org/apache/logging/log4j/core/util/internal/InternalLoggerRegistry.java create mode 100644 src/changelog/.2.x.x/3143_logger_registry.xml diff --git a/log4j-api-test/pom.xml b/log4j-api-test/pom.xml index e1fb806b9b4..aef8a1b5d64 100644 --- a/log4j-api-test/pom.xml +++ b/log4j-api-test/pom.xml @@ -144,6 +144,11 @@ mockito-inline test + + org.jspecify + jspecify + test + org.osgi 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(); @@ -196,7 +185,7 @@ public Collection getLoggers(final Collection destination) { */ public boolean hasLogger(final String name) { requireNonNull(name, "name"); - final T logger = getLogger(name); + final @Nullable T logger = getLogger(name); return logger != null; } @@ -215,7 +204,7 @@ public boolean hasLogger(final String name) { */ public boolean hasLogger(final String name, final MessageFactory messageFactory) { requireNonNull(name, "name"); - final T logger = getLogger(name, messageFactory); + final @Nullable T logger = getLogger(name, messageFactory); return logger != null; } @@ -232,7 +221,7 @@ public boolean hasLogger(final String name, final Class messageFactoryClass.equals(messageFactory.getClass())); } finally { readLock.unlock(); @@ -243,32 +232,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 */ - public void putIfAbsent(final String name, 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 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)); - } + 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 1748932083d..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.24.1") +@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 88f66ac364d..3369d50e62b 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; @@ -82,7 +83,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; @@ -512,7 +513,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()); } /** @@ -526,13 +527,7 @@ public Collection getLoggers() { public Logger getLogger(final String name, final MessageFactory messageFactory) { final MessageFactory effectiveMessageFactory = messageFactory != null ? messageFactory : DEFAULT_MESSAGE_FACTORY; - final Logger oldLogger = loggerRegistry.getLogger(name, effectiveMessageFactory); - if (oldLogger != null) { - return oldLogger; - } - final Logger newLogger = newInstance(name, effectiveMessageFactory); - loggerRegistry.putIfAbsent(name, effectiveMessageFactory, newLogger); - return loggerRegistry.getLogger(name, effectiveMessageFactory); + return loggerRegistry.computeIfAbsent(name, effectiveMessageFactory, this::newInstance); } /** @@ -540,9 +535,14 @@ public Logger getLogger(final String name, final MessageFactory messageFactory) * * @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; } /** @@ -775,9 +775,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/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`. + +