Expunge stale loggers in InternalLoggerRegistry on method invocation

log4j-core-test/src/main/java/org/apache/logging/log4j/core/test/util/InternalLoggerRegistryGCTest.java

@@ -0,0 +1,127 @@
+/*
+ * 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.test.util;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.junit.jupiter.api.Assertions.assertSame;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+import java.lang.ref.WeakReference;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import org.apache.logging.log4j.core.Logger;
+import org.apache.logging.log4j.core.LoggerContext;
+import org.apache.logging.log4j.core.util.internal.InternalLoggerRegistry;
+import org.apache.logging.log4j.message.MessageFactory;
+import org.apache.logging.log4j.message.SimpleMessageFactory;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+
+class InternalLoggerRegistryTest {
+    private InternalLoggerRegistry registry;
+    private MessageFactory messageFactory;
+
+    @BeforeEach
+    void setUp() {
+        registry = new InternalLoggerRegistry();
+        messageFactory = new SimpleMessageFactory();
+    }
+
+    @Test
+    void testGetLoggerReturnsNullForNonExistentLogger() {
+        assertNull(registry.getLogger("nonExistent", messageFactory));
+    }
+
+    @Test
+    void testComputeIfAbsentCreatesLogger() {
+        Logger logger =
+                registry.computeIfAbsent("testLogger", messageFactory, (name, factory) -> LoggerContext.getContext()
+                        .getLogger(name, factory));
+        assertNotNull(logger);
+        assertEquals("testLogger", logger.getName());
+    }
+
+    @Test
+    void testGetLoggerRetrievesExistingLogger() {
+        Logger logger =
+                registry.computeIfAbsent("testLogger", messageFactory, (name, factory) -> LoggerContext.getContext()
+                        .getLogger(name, factory));
+        assertSame(logger, registry.getLogger("testLogger", messageFactory));
+    }
+
+    @Test
+    void testHasLoggerReturnsCorrectStatus() {
+        assertFalse(registry.hasLogger("testLogger", messageFactory));
+        registry.computeIfAbsent("testLogger", messageFactory, (name, factory) -> LoggerContext.getContext()
+                .getLogger(name, factory));
+        assertTrue(registry.hasLogger("testLogger", messageFactory));
+    }
+
+    @Test
+    void testExpungeStaleEntriesRemovesGarbageCollectedLoggers() throws InterruptedException {
+        Logger logger =
+                registry.computeIfAbsent("testLogger", messageFactory, (name, factory) -> LoggerContext.getContext()
+                        .getLogger(name, factory));
+
+        WeakReference<Logger> weakRef = new WeakReference<>(logger);
+        logger = null; // Dereference to allow GC
+
+        // Retry loop to give GC time to collect
+        for (int i = 0; i < 10; i++) {
+            System.gc();
+            Thread.sleep(100);
+            if (weakRef.get() == null) {
+                break;
+            }
+        }
+
+        // Access the registry to potentially trigger cleanup
+        registry.computeIfAbsent("tempLogger", messageFactory, (name, factory) -> LoggerContext.getContext()
+                .getLogger(name, factory));
+
+        assertNull(weakRef.get(), "Logger should have been garbage collected");
+        assertNull(
+                registry.getLogger("testLogger", messageFactory), "Stale logger should be removed from the registry");
+    }
+
+    @Test
+    void testConcurrentAccess() throws InterruptedException {
+        int threadCount = 10;
+        ExecutorService executor = Executors.newFixedThreadPool(threadCount);
+        CountDownLatch latch = new CountDownLatch(threadCount);
+
+        for (int i = 0; i < threadCount; i++) {
+            executor.submit(() -> {
+                registry.computeIfAbsent("testLogger", messageFactory, (name, factory) -> LoggerContext.getContext()
+                        .getLogger(name, factory));
+                latch.countDown();
+            });
+        }
+
+        latch.await();
+        executor.shutdown();
+
+        // Verify logger was created and is accessible after concurrent creation
+        assertNotNull(
+                registry.getLogger("testLogger", messageFactory),
+                "Logger should be accessible after concurrent creation");
+    }
+}

log4j-core/src/main/java/org/apache/logging/log4j/core/util/internal/InternalLoggerRegistry.java

@@ -18,6 +18,8 @@
 
 import static java.util.Objects.requireNonNull;
 
+import java.lang.ref.Reference;
+import java.lang.ref.ReferenceQueue;
 import java.lang.ref.WeakReference;
 import java.util.Collection;
 import java.util.HashMap;
@@ -40,7 +42,9 @@
  * A registry of {@link Logger}s namespaced by name and message factory.
  * This class is internally used by {@link LoggerContext}.
  * <p>
- * We don't use {@linkplain org.apache.logging.log4j.spi.LoggerRegistry the registry from Log4j API} to keep Log4j Core independent from the version of Log4j API at runtime.
+ * We don't use {@linkplain org.apache.logging.log4j.spi.LoggerRegistry the
+ * registry from Log4j API} to keep Log4j Core independent from the version of
+ * Log4j API at runtime.
  * This also allows Log4j Core to evolve independently from Log4j API.
  * </p>
  *
@@ -53,23 +57,48 @@ public final class InternalLoggerRegistry {
             new WeakHashMap<>();
 
     private final ReadWriteLock lock = new ReentrantReadWriteLock();
-
     private final Lock readLock = lock.readLock();
-
     private final Lock writeLock = lock.writeLock();
 
+    // ReferenceQueue to track stale WeakReferences
+    private final ReferenceQueue<Logger> staleLoggerRefs = new ReferenceQueue<>();
+
     public InternalLoggerRegistry() {}
 
+    /**
+     * Expunges stale logger references from the registry.
+     */
+    private void expungeStaleEntries() {
+        Reference<? extends Logger> loggerRef;
+        while ((loggerRef = staleLoggerRefs.poll()) != null) {
+            removeLogger(loggerRef);
+        }
+    }
+
+    /**
+     * Removes a logger from the registry.
+     */
+    private void removeLogger(Reference<? extends Logger> loggerRef) {
+        writeLock.lock();
+        try {
+            loggerRefByNameByMessageFactory.values().forEach(map -> map.values().removeIf(ref -> ref == loggerRef));
+        } finally {
+            writeLock.unlock();
+        }
+    }
+
     /**
      * Returns the logger associated with the given name and message factory.
      *
-     * @param name a logger name
+     * @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");
+        expungeStaleEntries(); // Clean up before retrieving
+
         readLock.lock();
         try {
             final Map<String, WeakReference<Logger>> loggerRefByName =
@@ -87,6 +116,8 @@ public InternalLoggerRegistry() {}
     }
 
     public Collection<Logger> getLoggers() {
+        expungeStaleEntries(); // Clean up before retrieving
+
         readLock.lock();
         try {
             // Return a new collection to allow concurrent iteration over the loggers
@@ -107,7 +138,7 @@ public Collection<Logger> getLoggers() {
     /**
      * Checks if a logger associated with the given name and message factory exists.
      *
-     * @param name a logger name
+     * @param name           a logger name
      * @param messageFactory a message factory
      * @return {@code true}, if the logger exists; {@code false} otherwise.
      */
@@ -118,15 +149,18 @@ public boolean hasLogger(final String name, final MessageFactory messageFactory)
     }
 
     /**
-     * Checks if a logger associated with the given name and message factory type exists.
+     * Checks if a logger associated with the given name and message factory type
+     * exists.
      *
-     * @param name a logger name
+     * @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<? extends MessageFactory> messageFactoryClass) {
         requireNonNull(name, "name");
         requireNonNull(messageFactoryClass, "messageFactoryClass");
+        expungeStaleEntries(); // Clean up before checking
+
         readLock.lock();
         try {
             return loggerRefByNameByMessageFactory.entrySet().stream()
@@ -142,12 +176,12 @@ public Logger computeIfAbsent(
             final MessageFactory messageFactory,
             final BiFunction<String, MessageFactory, Logger> loggerSupplier) {
 
-        // Check arguments
         requireNonNull(name, "name");
         requireNonNull(messageFactory, "messageFactory");
         requireNonNull(loggerSupplier, "loggerSupplier");
 
-        // Read lock fast path: See if logger already exists
+        expungeStaleEntries(); // Clean up before adding a new logger
+
         @Nullable Logger logger = getLogger(name, messageFactory);
         if (logger != null) {
             return logger;
@@ -158,15 +192,19 @@ public Logger computeIfAbsent(
         // - Logger instantiation is expensive (causes contention on the write-lock)
         //
         // - User code might have circular code paths, though through different threads.
-        //   Consider `T1[ILR:computeIfAbsent] -> ... -> T1[Logger::new] -> ... -> T2[ILR::computeIfAbsent]`.
-        //   Hence, having logger instantiation while holding a write lock might cause deadlocks:
-        //   https://github.com/apache/logging-log4j2/issues/3252
-        //   https://github.com/apache/logging-log4j2/issues/3399
+        // Consider `T1[ILR:computeIfAbsent] -> ... -> T1[Logger::new] -> ... ->
+        // T2[ILR::computeIfAbsent]`.
+        // Hence, having logger instantiation while holding a write lock might cause
+        // deadlocks:
+        // https://github.com/apache/logging-log4j2/issues/3252
+        // https://github.com/apache/logging-log4j2/issues/3399
         //
-        // - Creating loggers without a lock, allows multiple threads to create loggers in parallel, which also improves
+        // - Creating loggers without a lock, allows multiple threads to create loggers
+        // in parallel, which also improves
         // performance.
         //
-        // Since all loggers with the same parameters are equivalent, we can safely return the logger from the
+        // Since all loggers with the same parameters are equivalent, we can safely
+        // return the logger from the
         // thread that finishes first.
         Logger newLogger = loggerSupplier.apply(name, messageFactory);
 
@@ -194,9 +232,10 @@ public Logger computeIfAbsent(
             if (loggerRefByName == null) {
                 loggerRefByNameByMessageFactory.put(messageFactory, loggerRefByName = new HashMap<>());
             }
+
             final WeakReference<Logger> loggerRef = loggerRefByName.get(name);
             if (loggerRef == null || (logger = loggerRef.get()) == null) {
-                loggerRefByName.put(name, new WeakReference<>(logger = newLogger));
+                loggerRefByName.put(name, new WeakReference<>(logger = newLogger, staleLoggerRefs));
             }
             return logger;
         } finally {