Removes weak references from LoggerRepository (2.24.x branch) (#3209)

This is a port of #3199 to the 2.24.x branch.

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.

Co-authored-by: Volkan Yazıcı <volkan@yazi.ci>

Author: ppkarwasz

log4j-api-test/pom.xml

@@ -144,6 +144,11 @@
       <artifactId>mockito-inline</artifactId>
       <scope>test</scope>
     </dependency>
+    <dependency>
+      <groupId>org.jspecify</groupId>
+      <artifactId>jspecify</artifactId>
+      <scope>test</scope>
+    </dependency>
     <!-- Used by ServiceLoaderUtilTest -->
     <dependency>
       <groupId>org.osgi</groupId>

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 <a href="https://github.com/apache/logging-log4j2/issues/3143>Issue #3143</a>
+     */
+    @ParameterizedTest
+    @MethodSource
+    void doesNotLoseLoggerReferences(@Nullable MessageFactory messageFactory) {
+        LoggerRegistry<TestLogger> loggerRegistry = new LoggerRegistry<>();
+        TestLogger logger = new TestLogger(LOGGER_NAME, messageFactory);
+        WeakReference<TestLogger> loggerRef = new WeakReference<>(logger);
+        // Register logger
+        loggerRegistry.putIfAbsent(LOGGER_NAME, messageFactory, logger);
+        // The JIT compiler/optimizer might figure out by himself the `logger` and `messageFactory` are no longer used:
+        //   https://shipilev.net/jvm/anatomy-quarks/8-local-var-reachability/
+        // We help him with the task though.
+        logger = null;
+        // Trigger a GC run
+        System.gc();
+        // Check if the logger is still there
+        assertThat(loggerRef.get()).isNotNull();
+        assertThat(loggerRegistry.getLogger(LOGGER_NAME, messageFactory)).isInstanceOf(TestLogger.class);
+    }
+}

log4j-api/src/main/java/org/apache/logging/log4j/spi/LoggerRegistry.java

@@ -18,27 +18,27 @@
 
 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.locks.Lock;
 import java.util.concurrent.locks.ReadWriteLock;
 import java.util.concurrent.locks.ReentrantReadWriteLock;
+import org.apache.logging.log4j.Logger;
 import org.apache.logging.log4j.message.MessageFactory;
 import org.apache.logging.log4j.message.ParameterizedMessageFactory;
+import org.jspecify.annotations.Nullable;
 
 /**
  * Convenience class to be used as an {@link ExtendedLogger} registry by {@code LoggerContext} implementations.
  */
 public class LoggerRegistry<T extends ExtendedLogger> {
 
-    private final Map<String, Map<MessageFactory, WeakReference<T>>> loggerRefByMessageFactoryByName = new HashMap<>();
+    private final Map<String, Map<MessageFactory, T>> loggerByMessageFactoryByName = new HashMap<>();
 
     private final ReadWriteLock lock = new ReentrantReadWriteLock();
 
@@ -127,7 +127,7 @@ public LoggerRegistry(final MapFactory<T> mapFactory) {
      * @param name a logger name
      * @return the logger associated with the name
      */
-    public T getLogger(final String name) {
+    public @Nullable T getLogger(final String name) {
         requireNonNull(name, "name");
         return getLogger(name, null);
     }
@@ -144,39 +144,29 @@ public T getLogger(final String 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) {
+    public @Nullable T getLogger(final String name, @Nullable final MessageFactory messageFactory) {
         requireNonNull(name, "name");
         readLock.lock();
         try {
-            final Map<MessageFactory, WeakReference<T>> loggerRefByMessageFactory =
-                    loggerRefByMessageFactoryByName.get(name);
-            if (loggerRefByMessageFactory == null) {
-                return null;
-            }
+            final @Nullable Map<MessageFactory, T> loggerByMessageFactory = loggerByMessageFactoryByName.get(name);
             final MessageFactory effectiveMessageFactory =
                     messageFactory != null ? messageFactory : ParameterizedMessageFactory.INSTANCE;
-            final WeakReference<T> loggerRef = loggerRefByMessageFactory.get(effectiveMessageFactory);
-            if (loggerRef == null) {
-                return null;
-            }
-            return loggerRef.get();
+            return loggerByMessageFactory == null ? null : loggerByMessageFactory.get(effectiveMessageFactory);
         } finally {
             readLock.unlock();
         }
     }
 
     public Collection<T> getLoggers() {
-        return getLoggers(new ArrayList<T>());
+        return getLoggers(new ArrayList<>());
     }
 
     public Collection<T> getLoggers(final Collection<T> 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 +186,7 @@ public Collection<T> getLoggers(final Collection<T> 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 +205,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 +222,7 @@ public boolean hasLogger(final String name, final Class<? extends MessageFactory
         requireNonNull(messageFactoryClass, "messageFactoryClass");
         readLock.lock();
         try {
-            return loggerRefByMessageFactoryByName.getOrDefault(name, Collections.emptyMap()).keySet().stream()
+            return loggerByMessageFactoryByName.getOrDefault(name, Collections.emptyMap()).keySet().stream()
                     .anyMatch(messageFactory -> messageFactoryClass.equals(messageFactory.getClass()));
         } finally {
             readLock.unlock();
@@ -241,34 +231,42 @@ public boolean hasLogger(final String name, final Class<? extends MessageFactory
 
     /**
      * Registers the provided logger.
-     * <b>Logger name and message factory parameters are ignored</b>, those will be obtained from the logger instead.
+     * <p>
+     *   The logger will be registered using the keys provided by the {@code name} and {@code messageFactory} parameters
+     *   and the values of {@link Logger#getName()} and {@link Logger#getMessageFactory()}.
+     * </p>
      *
-     * @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<MessageFactory, WeakReference<T>> loggerRefByMessageFactory =
-                    loggerRefByMessageFactoryByName.computeIfAbsent(
-                            logger.getName(), this::createLoggerRefByMessageFactoryMap);
-            final MessageFactory loggerMessageFactory = logger.getMessageFactory();
-            final WeakReference<T> loggerRef = loggerRefByMessageFactory.get(loggerMessageFactory);
-            if (loggerRef == null || loggerRef.get() == null) {
-                loggerRefByMessageFactory.put(loggerMessageFactory, new WeakReference<>(logger));
+            final MessageFactory effectiveMessageFactory =
+                    messageFactory != null ? messageFactory : ParameterizedMessageFactory.INSTANCE;
+            // Register using the keys provided by the caller
+            loggerByMessageFactoryByName
+                    .computeIfAbsent(name, this::createLoggerRefByMessageFactoryMap)
+                    .putIfAbsent(effectiveMessageFactory, logger);
+            // Also register using the values extracted from `logger`
+            if (!name.equals(logger.getName()) || !effectiveMessageFactory.equals(logger.getMessageFactory())) {
+                loggerByMessageFactoryByName
+                        .computeIfAbsent(logger.getName(), this::createLoggerRefByMessageFactoryMap)
+                        .putIfAbsent(logger.getMessageFactory(), logger);
             }
         } finally {
             writeLock.unlock();
         }
     }
 
-    private Map<MessageFactory, WeakReference<T>> createLoggerRefByMessageFactoryMap(final String ignored) {
+    private Map<MessageFactory, T> createLoggerRefByMessageFactoryMap(final String ignored) {
         return new WeakHashMap<>();
     }
 }

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;

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<Logger> loggerRegistry = new LoggerRegistry<>();
+    private final InternalLoggerRegistry loggerRegistry = new InternalLoggerRegistry();
     private final CopyOnWriteArrayList<PropertyChangeListener> propertyChangeListeners = new CopyOnWriteArrayList<>();
     private volatile List<LoggerContextShutdownAware> listeners;
 
@@ -512,7 +513,7 @@ public Logger getLogger(final String name) {
      * @return a collection of the current loggers.
      */
     public Collection<Logger> getLoggers() {
-        return loggerRegistry.getLoggers();
+        return loggerRegistry.getLoggers().collect(Collectors.toList());
     }
 
     /**
@@ -526,13 +527,7 @@ public Collection<Logger> 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);
     }
 
     /**
@@ -541,8 +536,11 @@ public Logger getLogger(final String name, final MessageFactory messageFactory)
      * @return the LoggerRegistry.
      * @since 2.17.2
      */
-    public LoggerRegistry<Logger> getLoggerRegistry() {
-        return loggerRegistry;
+    public org.apache.logging.log4j.spi.LoggerRegistry<Logger> getLoggerRegistry() {
+        org.apache.logging.log4j.spi.LoggerRegistry<Logger> result =
+                new org.apache.logging.log4j.spi.LoggerRegistry<>();
+        loggerRegistry.getLoggers().forEach(l -> result.putIfAbsent(l.getName(), l.getMessageFactory(), l));
+        return result;
     }
 
     /**
@@ -775,9 +773,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));
     }