From 04facd72ae464deaaad936b440bca3f9232e6824 Mon Sep 17 00:00:00 2001 From: suvrat1629 Date: Wed, 19 Feb 2025 20:10:15 +0530 Subject: [PATCH 1/9] ensured stale loggers are removed from InternalLoggerRegistry and also added InternalLoggerRegistryGCTest --- .../util/InternalLoggerRegistryGCTest.java | 122 ++++++++++++++++++ .../util/internal/InternalLoggerRegistry.java | 89 ++++++------- 2 files changed, 162 insertions(+), 49 deletions(-) create mode 100644 log4j-core-test/src/main/java/org/apache/logging/log4j/core/test/util/InternalLoggerRegistryGCTest.java diff --git a/log4j-core-test/src/main/java/org/apache/logging/log4j/core/test/util/InternalLoggerRegistryGCTest.java b/log4j-core-test/src/main/java/org/apache/logging/log4j/core/test/util/InternalLoggerRegistryGCTest.java new file mode 100644 index 00000000000..c913f923368 --- /dev/null +++ b/log4j-core-test/src/main/java/org/apache/logging/log4j/core/test/util/InternalLoggerRegistryGCTest.java @@ -0,0 +1,122 @@ +/* + * 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.*; + +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 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"); + } +} 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 index eff1d46b77a..a1926a6d61f 100644 --- 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 @@ -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; @@ -39,10 +41,8 @@ /** * A registry of {@link Logger}s namespaced by name and message factory. * This class is internally used by {@link LoggerContext}. - *

- * 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. - *

+ * + * Handles automatic cleanup of stale logger references to prevent memory leaks. * * @since 2.25.0 */ @@ -53,23 +53,44 @@ 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 staleLoggerRefs = new ReferenceQueue<>(); + public InternalLoggerRegistry() {} + /** + * Expunges stale logger references from the registry. + */ + private void expungeStaleEntries() { + Reference loggerRef; + while ((loggerRef = staleLoggerRefs.poll()) != null) { + removeLogger(loggerRef); + } + } + + /** + * Removes a logger from the registry. + */ + private void removeLogger(Reference 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 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> loggerRefByName = @@ -87,11 +108,10 @@ public InternalLoggerRegistry() {} } public Collection getLoggers() { + expungeStaleEntries(); // Clean up before retrieving + readLock.lock(); try { - // Return a new collection to allow concurrent iteration over the loggers - // - // https://github.com/apache/logging-log4j2/issues/3234 return loggerRefByNameByMessageFactory.values().stream() .flatMap(loggerRefByName -> loggerRefByName.values().stream()) .flatMap(loggerRef -> { @@ -104,29 +124,17 @@ public Collection getLoggers() { } } - /** - * 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"); + expungeStaleEntries(); // Clean up before checking + readLock.lock(); try { return loggerRefByNameByMessageFactory.entrySet().stream() @@ -142,37 +150,21 @@ public Logger computeIfAbsent( 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 + expungeStaleEntries(); // Clean up before adding a new logger + @Nullable Logger logger = getLogger(name, messageFactory); if (logger != null) { return logger; } - // Intentionally moving the logger creation outside the write lock, because: - // - // - 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 - // - // - 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 - // thread that finishes first. Logger newLogger = loggerSupplier.apply(name, messageFactory); - - // Report name and message factory mismatch if there are any final String loggerName = newLogger.getName(); final MessageFactory loggerMessageFactory = newLogger.getMessageFactory(); + if (!loggerName.equals(name) || !loggerMessageFactory.equals(messageFactory)) { StatusLogger.getLogger() .error( @@ -186,17 +178,16 @@ public Logger computeIfAbsent( messageFactory); } - // Write lock slow path: Insert the logger writeLock.lock(); try { Map> loggerRefByName = loggerRefByNameByMessageFactory.get(messageFactory); - // noinspection Java8MapApi (avoid the allocation of lambda passed to `Map::computeIfAbsent`) if (loggerRefByName == null) { loggerRefByNameByMessageFactory.put(messageFactory, loggerRefByName = new HashMap<>()); } + final WeakReference 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 { From f53251c7e60ab5149e8de9b7efebe519fd30808b Mon Sep 17 00:00:00 2001 From: suvrat1629 Date: Wed, 19 Feb 2025 20:21:21 +0530 Subject: [PATCH 2/9] removed wildcard imports --- .../log4j/core/test/util/InternalLoggerRegistryGCTest.java | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/log4j-core-test/src/main/java/org/apache/logging/log4j/core/test/util/InternalLoggerRegistryGCTest.java b/log4j-core-test/src/main/java/org/apache/logging/log4j/core/test/util/InternalLoggerRegistryGCTest.java index c913f923368..59f7bdd0b68 100644 --- a/log4j-core-test/src/main/java/org/apache/logging/log4j/core/test/util/InternalLoggerRegistryGCTest.java +++ b/log4j-core-test/src/main/java/org/apache/logging/log4j/core/test/util/InternalLoggerRegistryGCTest.java @@ -16,7 +16,12 @@ */ package org.apache.logging.log4j.core.test.util; -import static org.junit.jupiter.api.Assertions.*; +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; From 207a7c35142db8d05c3851d0a9559e096ebcac05 Mon Sep 17 00:00:00 2001 From: suvrat1629 Date: Fri, 21 Feb 2025 19:39:31 +0530 Subject: [PATCH 3/9] added the comments again --- .../util/internal/InternalLoggerRegistry.java | 54 +++++++++++++++++-- 1 file changed, 51 insertions(+), 3 deletions(-) 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 index a1926a6d61f..af641684bc2 100644 --- 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 @@ -41,8 +41,12 @@ /** * A registry of {@link Logger}s namespaced by name and message factory. * This class is internally used by {@link LoggerContext}. - * - * Handles automatic cleanup of stale logger references to prevent memory leaks. + *

+ * 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. + *

* * @since 2.25.0 */ @@ -85,6 +89,10 @@ private void removeLogger(Reference loggerRef) { /** * 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"); @@ -112,6 +120,9 @@ public Collection getLoggers() { readLock.lock(); try { + // Return a new collection to allow concurrent iteration over the loggers + // + // https://github.com/apache/logging-log4j2/issues/3234 return loggerRefByNameByMessageFactory.values().stream() .flatMap(loggerRefByName -> loggerRefByName.values().stream()) .flatMap(loggerRef -> { @@ -124,12 +135,27 @@ public Collection getLoggers() { } } + /** + * 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"); @@ -161,10 +187,30 @@ public Logger computeIfAbsent( return logger; } + // Intentionally moving the logger creation outside the write lock, because: + // + // - 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 + // + // - 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 + // thread that finishes first. Logger newLogger = loggerSupplier.apply(name, messageFactory); + + // Report name and message factory mismatch if there are any final String loggerName = newLogger.getName(); final MessageFactory loggerMessageFactory = newLogger.getMessageFactory(); - if (!loggerName.equals(name) || !loggerMessageFactory.equals(messageFactory)) { StatusLogger.getLogger() .error( @@ -178,9 +224,11 @@ public Logger computeIfAbsent( messageFactory); } + // Write lock slow path: Insert the logger writeLock.lock(); try { Map> loggerRefByName = loggerRefByNameByMessageFactory.get(messageFactory); + // noinspection Java8MapApi (avoid the allocation of lambda passed to `Map::computeIfAbsent`) if (loggerRefByName == null) { loggerRefByNameByMessageFactory.put(messageFactory, loggerRefByName = new HashMap<>()); } From 9a1d8e3e53223f55df1287ec76a4fa1c19c76f5d Mon Sep 17 00:00:00 2001 From: suvrat1629 Date: Fri, 21 Feb 2025 21:28:38 +0530 Subject: [PATCH 4/9] changed the removeLogger to not iterate --- .../core/util/internal/InternalLoggerRegistry.java | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) 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 index af641684bc2..d379380e575 100644 --- 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 @@ -79,9 +79,21 @@ private void expungeStaleEntries() { * Removes a logger from the registry. */ private void removeLogger(Reference loggerRef) { + Logger logger = loggerRef.get(); + if (logger == null) return; // Logger already cleared + + MessageFactory messageFactory = logger.getMessageFactory(); + String name = logger.getName(); + writeLock.lock(); try { - loggerRefByNameByMessageFactory.values().forEach(map -> map.values().removeIf(ref -> ref == loggerRef)); + Map> loggerRefByName = loggerRefByNameByMessageFactory.get(messageFactory); + if (loggerRefByName != null) { + loggerRefByName.remove(name); + if (loggerRefByName.isEmpty()) { + loggerRefByNameByMessageFactory.remove(messageFactory); // Cleanup + } + } } finally { writeLock.unlock(); } From 05b3f8f0cafdcc8bf395af655d82560b3bf1d764 Mon Sep 17 00:00:00 2001 From: suvrat1629 Date: Sun, 23 Feb 2025 10:12:46 +0530 Subject: [PATCH 5/9] reverted more changes --- .../util/internal/InternalLoggerRegistry.java | 27 +++++++++---------- 1 file changed, 12 insertions(+), 15 deletions(-) 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 index d379380e575..a83faa830d1 100644 --- 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 @@ -102,7 +102,7 @@ private void removeLogger(Reference loggerRef) { /** * 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 */ @@ -150,7 +150,7 @@ public Collection 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. */ @@ -161,10 +161,9 @@ 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. */ @@ -188,12 +187,14 @@ public Logger computeIfAbsent( final MessageFactory messageFactory, final BiFunction loggerSupplier) { + // Check arguments requireNonNull(name, "name"); requireNonNull(messageFactory, "messageFactory"); requireNonNull(loggerSupplier, "loggerSupplier"); expungeStaleEntries(); // Clean up before adding a new logger + // Read lock fast path: See if logger already exists @Nullable Logger logger = getLogger(name, messageFactory); if (logger != null) { return logger; @@ -204,19 +205,15 @@ 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); From 8b964a0d65c71c0b384ce46d68a86c17741eb580 Mon Sep 17 00:00:00 2001 From: Junhyeok Lee Date: Wed, 21 May 2025 22:58:48 +0900 Subject: [PATCH 6/9] Expunge stale entries in InternalLoggerRegistry Use ReferenceQueue to track and remove GC'd Logger WeakReferences from the registry map to prevent potential memory leaks. Includes improved unit tests. --- .../util/InternalLoggerRegistryGCTest.java | 117 +++++++++++++----- .../util/internal/InternalLoggerRegistry.java | 44 +++---- 2 files changed, 105 insertions(+), 56 deletions(-) diff --git a/log4j-core-test/src/main/java/org/apache/logging/log4j/core/test/util/InternalLoggerRegistryGCTest.java b/log4j-core-test/src/main/java/org/apache/logging/log4j/core/test/util/InternalLoggerRegistryGCTest.java index 59f7bdd0b68..0888a08522c 100644 --- a/log4j-core-test/src/main/java/org/apache/logging/log4j/core/test/util/InternalLoggerRegistryGCTest.java +++ b/log4j-core-test/src/main/java/org/apache/logging/log4j/core/test/util/InternalLoggerRegistryGCTest.java @@ -16,6 +16,9 @@ */ package org.apache.logging.log4j.core.test.util; +import static java.util.concurrent.TimeUnit.MILLISECONDS; +import static java.util.concurrent.TimeUnit.SECONDS; +import static org.awaitility.Awaitility.await; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNotNull; @@ -24,6 +27,8 @@ import static org.junit.jupiter.api.Assertions.assertTrue; import java.lang.ref.WeakReference; +import java.lang.reflect.Field; +import java.util.Map; import java.util.concurrent.CountDownLatch; import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; @@ -32,17 +37,30 @@ 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.AfterEach; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.TestInfo; class InternalLoggerRegistryTest { + private LoggerContext loggerContext; private InternalLoggerRegistry registry; private MessageFactory messageFactory; @BeforeEach - void setUp() { - registry = new InternalLoggerRegistry(); - messageFactory = new SimpleMessageFactory(); + void setUp(TestInfo testInfo) throws NoSuchFieldException, IllegalAccessException { + loggerContext = new LoggerContext(testInfo.getDisplayName()); + Field registryField = loggerContext.getClass().getDeclaredField("loggerRegistry"); + registryField.setAccessible(true); + registry = (InternalLoggerRegistry) registryField.get(loggerContext); + messageFactory = SimpleMessageFactory.INSTANCE; + } + + @AfterEach + void tearDown() { + if (loggerContext != null) { + loggerContext.stop(); + } } @Test @@ -52,54 +70,79 @@ void testGetLoggerReturnsNullForNonExistentLogger() { @Test void testComputeIfAbsentCreatesLogger() { - Logger logger = - registry.computeIfAbsent("testLogger", messageFactory, (name, factory) -> LoggerContext.getContext() - .getLogger(name, factory)); + Logger logger = registry.computeIfAbsent( + "testLogger", messageFactory, (name, factory) -> loggerContext.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)); + Logger logger = registry.computeIfAbsent( + "testLogger", messageFactory, (name, factory) -> loggerContext.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)); + registry.computeIfAbsent( + "testLogger", messageFactory, (name, factory) -> loggerContext.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 weakRef = new WeakReference<>(logger); - logger = null; // Dereference to allow GC + void testExpungeStaleWeakReferenceEntries() { + String loggerNamePrefix = "testLogger_"; + int numberOfLoggers = 1000; + + for (int i = 0; i < numberOfLoggers; i++) { + Logger logger = registry.computeIfAbsent( + loggerNamePrefix + i, messageFactory, (name, factory) -> loggerContext.getLogger(name, factory)); + logger.info("Using logger {}", logger.getName()); + } - // Retry loop to give GC time to collect - for (int i = 0; i < 10; i++) { + await().atMost(10, SECONDS).pollInterval(100, MILLISECONDS).untilAsserted(() -> { System.gc(); - Thread.sleep(100); - if (weakRef.get() == null) { - break; + System.runFinalization(); + registry.computeIfAbsent( + "triggerExpunge", messageFactory, (name, factory) -> loggerContext.getLogger(name, factory)); + + Map>> loggerRefByNameByMessageFactory = + reflectAndGetLoggerMapFromRegistry(); + Map> loggerRefByName = loggerRefByNameByMessageFactory.get(messageFactory); + + boolean isExpungeStaleEntries = true; + for (int i = 0; i < numberOfLoggers; i++) { + if (loggerRefByName.containsKey(loggerNamePrefix + i)) { + isExpungeStaleEntries = false; + break; + } } - } + assertTrue( + isExpungeStaleEntries, + "Stale WeakReference entries were not removed from the inner map for MessageFactory"); + }); + } - // Access the registry to potentially trigger cleanup - registry.computeIfAbsent("tempLogger", messageFactory, (name, factory) -> LoggerContext.getContext() - .getLogger(name, factory)); + @Test + void testExpungeStaleMessageFactoryEntry() { + Logger logger = registry.computeIfAbsent( + "testLogger", messageFactory, (name, factory) -> loggerContext.getLogger(name, factory)); + logger.info("Using logger {}", logger.getName()); + logger = null; - assertNull(weakRef.get(), "Logger should have been garbage collected"); - assertNull( - registry.getLogger("testLogger", messageFactory), "Stale logger should be removed from the registry"); + await().atMost(10, SECONDS).pollInterval(100, MILLISECONDS).untilAsserted(() -> { + System.gc(); + System.runFinalization(); + registry.getLogger("testLogger", messageFactory); + + Map>> loggerRefByNameByMessageFactory = + reflectAndGetLoggerMapFromRegistry(); + assertNull( + loggerRefByNameByMessageFactory.get(messageFactory), + "Stale MessageFactory entry was not removed from the outer map"); + }); } @Test @@ -110,8 +153,8 @@ void testConcurrentAccess() throws InterruptedException { for (int i = 0; i < threadCount; i++) { executor.submit(() -> { - registry.computeIfAbsent("testLogger", messageFactory, (name, factory) -> LoggerContext.getContext() - .getLogger(name, factory)); + registry.computeIfAbsent( + "testLogger", messageFactory, (name, factory) -> loggerContext.getLogger(name, factory)); latch.countDown(); }); } @@ -124,4 +167,14 @@ void testConcurrentAccess() throws InterruptedException { registry.getLogger("testLogger", messageFactory), "Logger should be accessible after concurrent creation"); } + + private Map>> reflectAndGetLoggerMapFromRegistry() + throws NoSuchFieldException, IllegalAccessException { + Field loggerMapField = registry.getClass().getDeclaredField("loggerRefByNameByMessageFactory"); + loggerMapField.setAccessible(true); + @SuppressWarnings("unchecked") + Map>> loggerMap = + (Map>>) loggerMapField.get(registry); + return loggerMap; + } } 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 index a83faa830d1..6647285da97 100644 --- 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 @@ -23,6 +23,7 @@ import java.lang.ref.WeakReference; import java.util.Collection; import java.util.HashMap; +import java.util.Iterator; import java.util.Map; import java.util.WeakHashMap; import java.util.concurrent.locks.Lock; @@ -66,36 +67,31 @@ public final class InternalLoggerRegistry { public InternalLoggerRegistry() {} /** - * Expunges stale logger references from the registry. + * Expunges stale entries for logger references and message factories. */ private void expungeStaleEntries() { - Reference loggerRef; - while ((loggerRef = staleLoggerRefs.poll()) != null) { - removeLogger(loggerRef); - } - } - - /** - * Removes a logger from the registry. - */ - private void removeLogger(Reference loggerRef) { - Logger logger = loggerRef.get(); - if (logger == null) return; // Logger already cleared + Reference loggerRef = staleLoggerRefs.poll(); - MessageFactory messageFactory = logger.getMessageFactory(); - String name = logger.getName(); + if (loggerRef != null) { + writeLock.lock(); + try { + while (staleLoggerRefs.poll() != null) { + // Clear refQueue + } - writeLock.lock(); - try { - Map> loggerRefByName = loggerRefByNameByMessageFactory.get(messageFactory); - if (loggerRefByName != null) { - loggerRefByName.remove(name); - if (loggerRefByName.isEmpty()) { - loggerRefByNameByMessageFactory.remove(messageFactory); // Cleanup + Iterator>>> outerIt = + loggerRefByNameByMessageFactory.entrySet().iterator(); + while (outerIt.hasNext()) { + Map.Entry>> outerEntry = outerIt.next(); + Map> innerMap = outerEntry.getValue(); + innerMap.values().removeIf(weakRef -> weakRef.get() == null); + if (innerMap.isEmpty()) { + outerIt.remove(); + } } + } finally { + writeLock.unlock(); } - } finally { - writeLock.unlock(); } } From b432d1458cff3c4ea58f209b4fdc3b41460bb862 Mon Sep 17 00:00:00 2001 From: Junhyeok Lee Date: Fri, 23 May 2025 22:01:10 +0900 Subject: [PATCH 7/9] Updated InternalLoggerRegistryTest * Adjusted test class file name and its package location. * Used MockLogger in logger suppliers to avoid nested computeIfAbsent. * Employed a local SimpleMessageFactory instance in tests. --- .../internal/InternalLoggerRegistryTest.java} | 31 ++++++++++++------- 1 file changed, 20 insertions(+), 11 deletions(-) rename log4j-core-test/src/{main/java/org/apache/logging/log4j/core/test/util/InternalLoggerRegistryGCTest.java => test/java/org/apache/logging/log4j/core/util/internal/InternalLoggerRegistryTest.java} (87%) diff --git a/log4j-core-test/src/main/java/org/apache/logging/log4j/core/test/util/InternalLoggerRegistryGCTest.java b/log4j-core-test/src/test/java/org/apache/logging/log4j/core/util/internal/InternalLoggerRegistryTest.java similarity index 87% rename from log4j-core-test/src/main/java/org/apache/logging/log4j/core/test/util/InternalLoggerRegistryGCTest.java rename to log4j-core-test/src/test/java/org/apache/logging/log4j/core/util/internal/InternalLoggerRegistryTest.java index 0888a08522c..b63edfe3b71 100644 --- a/log4j-core-test/src/main/java/org/apache/logging/log4j/core/test/util/InternalLoggerRegistryGCTest.java +++ b/log4j-core-test/src/test/java/org/apache/logging/log4j/core/util/internal/InternalLoggerRegistryTest.java @@ -14,7 +14,7 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -package org.apache.logging.log4j.core.test.util; +package org.apache.logging.log4j.core.util.internal; import static java.util.concurrent.TimeUnit.MILLISECONDS; import static java.util.concurrent.TimeUnit.SECONDS; @@ -34,7 +34,6 @@ 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.AfterEach; @@ -71,7 +70,7 @@ void testGetLoggerReturnsNullForNonExistentLogger() { @Test void testComputeIfAbsentCreatesLogger() { Logger logger = registry.computeIfAbsent( - "testLogger", messageFactory, (name, factory) -> loggerContext.getLogger(name, factory)); + "testLogger", messageFactory, (name, factory) -> new MockLogger(loggerContext, name, factory)); assertNotNull(logger); assertEquals("testLogger", logger.getName()); } @@ -79,7 +78,7 @@ void testComputeIfAbsentCreatesLogger() { @Test void testGetLoggerRetrievesExistingLogger() { Logger logger = registry.computeIfAbsent( - "testLogger", messageFactory, (name, factory) -> loggerContext.getLogger(name, factory)); + "testLogger", messageFactory, (name, factory) -> new MockLogger(loggerContext, name, factory)); assertSame(logger, registry.getLogger("testLogger", messageFactory)); } @@ -87,7 +86,7 @@ void testGetLoggerRetrievesExistingLogger() { void testHasLoggerReturnsCorrectStatus() { assertFalse(registry.hasLogger("testLogger", messageFactory)); registry.computeIfAbsent( - "testLogger", messageFactory, (name, factory) -> loggerContext.getLogger(name, factory)); + "testLogger", messageFactory, (name, factory) -> new MockLogger(loggerContext, name, factory)); assertTrue(registry.hasLogger("testLogger", messageFactory)); } @@ -98,7 +97,9 @@ void testExpungeStaleWeakReferenceEntries() { for (int i = 0; i < numberOfLoggers; i++) { Logger logger = registry.computeIfAbsent( - loggerNamePrefix + i, messageFactory, (name, factory) -> loggerContext.getLogger(name, factory)); + loggerNamePrefix + i, + messageFactory, + (name, factory) -> new MockLogger(loggerContext, name, factory)); logger.info("Using logger {}", logger.getName()); } @@ -106,7 +107,7 @@ void testExpungeStaleWeakReferenceEntries() { System.gc(); System.runFinalization(); registry.computeIfAbsent( - "triggerExpunge", messageFactory, (name, factory) -> loggerContext.getLogger(name, factory)); + "triggerExpunge", messageFactory, (name, factory) -> new MockLogger(loggerContext, name, factory)); Map>> loggerRefByNameByMessageFactory = reflectAndGetLoggerMapFromRegistry(); @@ -127,20 +128,21 @@ void testExpungeStaleWeakReferenceEntries() { @Test void testExpungeStaleMessageFactoryEntry() { + SimpleMessageFactory mockMessageFactory = new SimpleMessageFactory(); Logger logger = registry.computeIfAbsent( - "testLogger", messageFactory, (name, factory) -> loggerContext.getLogger(name, factory)); + "testLogger", mockMessageFactory, (name, factory) -> new MockLogger(loggerContext, name, factory)); logger.info("Using logger {}", logger.getName()); logger = null; await().atMost(10, SECONDS).pollInterval(100, MILLISECONDS).untilAsserted(() -> { System.gc(); System.runFinalization(); - registry.getLogger("testLogger", messageFactory); + registry.getLogger("triggerExpunge", mockMessageFactory); Map>> loggerRefByNameByMessageFactory = reflectAndGetLoggerMapFromRegistry(); assertNull( - loggerRefByNameByMessageFactory.get(messageFactory), + loggerRefByNameByMessageFactory.get(mockMessageFactory), "Stale MessageFactory entry was not removed from the outer map"); }); } @@ -154,7 +156,7 @@ void testConcurrentAccess() throws InterruptedException { for (int i = 0; i < threadCount; i++) { executor.submit(() -> { registry.computeIfAbsent( - "testLogger", messageFactory, (name, factory) -> loggerContext.getLogger(name, factory)); + "testLogger", messageFactory, (name, factory) -> new MockLogger(loggerContext, name, factory)); latch.countDown(); }); } @@ -177,4 +179,11 @@ private Map>> reflectAndGetLog (Map>>) loggerMapField.get(registry); return loggerMap; } + + private class MockLogger extends Logger { + + protected MockLogger(LoggerContext context, String name, MessageFactory messageFactory) { + super(context, name, messageFactory); + } + } } From 0cde993f38f8f7a5ade20c510a3229f57de9c022 Mon Sep 17 00:00:00 2001 From: Junhyeok Lee Date: Sat, 24 May 2025 10:59:20 +0900 Subject: [PATCH 8/9] Add changelog entry --- ...0_InternalLoggerRegistry_stale_entry_expunge.xml | 13 +++++++++++++ 1 file changed, 13 insertions(+) create mode 100644 src/changelog/.2.x.x/3430_InternalLoggerRegistry_stale_entry_expunge.xml diff --git a/src/changelog/.2.x.x/3430_InternalLoggerRegistry_stale_entry_expunge.xml b/src/changelog/.2.x.x/3430_InternalLoggerRegistry_stale_entry_expunge.xml new file mode 100644 index 00000000000..75c40209783 --- /dev/null +++ b/src/changelog/.2.x.x/3430_InternalLoggerRegistry_stale_entry_expunge.xml @@ -0,0 +1,13 @@ + + + + + + Improved expunging of stale entries in `InternalLoggerRegistry` to prevent potential memory leaks + + From 3df581e9eb687888c77e1a50338671410e111372 Mon Sep 17 00:00:00 2001 From: Junhyeok Lee Date: Thu, 29 May 2025 18:32:03 +0900 Subject: [PATCH 9/9] Update InternalLoggerRegistry and tests * Rename variables in InternalLoggerRegistry * Add final keyword in InternalLoggerRegistry and tests * Remove/clarify comments in InternalLoggerRegistry * Remove MockLogger class, using anonymous Logger instances in tests * Remove System.runFinalization() calls from tests * Improve assertion logic in testExpungeStaleWeakReferenceEntries * Remove testConcurrentAccess (lacked specific scenario) --- .../internal/InternalLoggerRegistryTest.java | 82 ++++++------------- .../util/internal/InternalLoggerRegistry.java | 30 +++---- 2 files changed, 39 insertions(+), 73 deletions(-) diff --git a/log4j-core-test/src/test/java/org/apache/logging/log4j/core/util/internal/InternalLoggerRegistryTest.java b/log4j-core-test/src/test/java/org/apache/logging/log4j/core/util/internal/InternalLoggerRegistryTest.java index b63edfe3b71..81df39b24b9 100644 --- a/log4j-core-test/src/test/java/org/apache/logging/log4j/core/util/internal/InternalLoggerRegistryTest.java +++ b/log4j-core-test/src/test/java/org/apache/logging/log4j/core/util/internal/InternalLoggerRegistryTest.java @@ -29,9 +29,6 @@ import java.lang.ref.WeakReference; import java.lang.reflect.Field; import java.util.Map; -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.message.MessageFactory; @@ -49,7 +46,7 @@ class InternalLoggerRegistryTest { @BeforeEach void setUp(TestInfo testInfo) throws NoSuchFieldException, IllegalAccessException { loggerContext = new LoggerContext(testInfo.getDisplayName()); - Field registryField = loggerContext.getClass().getDeclaredField("loggerRegistry"); + final Field registryField = loggerContext.getClass().getDeclaredField("loggerRegistry"); registryField.setAccessible(true); registry = (InternalLoggerRegistry) registryField.get(loggerContext); messageFactory = SimpleMessageFactory.INSTANCE; @@ -69,16 +66,16 @@ void testGetLoggerReturnsNullForNonExistentLogger() { @Test void testComputeIfAbsentCreatesLogger() { - Logger logger = registry.computeIfAbsent( - "testLogger", messageFactory, (name, factory) -> new MockLogger(loggerContext, name, factory)); + final Logger logger = registry.computeIfAbsent( + "testLogger", messageFactory, (name, factory) -> new Logger(loggerContext, name, factory) {}); assertNotNull(logger); assertEquals("testLogger", logger.getName()); } @Test void testGetLoggerRetrievesExistingLogger() { - Logger logger = registry.computeIfAbsent( - "testLogger", messageFactory, (name, factory) -> new MockLogger(loggerContext, name, factory)); + final Logger logger = registry.computeIfAbsent( + "testLogger", messageFactory, (name, factory) -> new Logger(loggerContext, name, factory) {}); assertSame(logger, registry.getLogger("testLogger", messageFactory)); } @@ -86,60 +83,57 @@ void testGetLoggerRetrievesExistingLogger() { void testHasLoggerReturnsCorrectStatus() { assertFalse(registry.hasLogger("testLogger", messageFactory)); registry.computeIfAbsent( - "testLogger", messageFactory, (name, factory) -> new MockLogger(loggerContext, name, factory)); + "testLogger", messageFactory, (name, factory) -> new Logger(loggerContext, name, factory) {}); assertTrue(registry.hasLogger("testLogger", messageFactory)); } @Test void testExpungeStaleWeakReferenceEntries() { - String loggerNamePrefix = "testLogger_"; - int numberOfLoggers = 1000; + final String loggerNamePrefix = "testLogger_"; + final int numberOfLoggers = 1000; for (int i = 0; i < numberOfLoggers; i++) { - Logger logger = registry.computeIfAbsent( + final Logger logger = registry.computeIfAbsent( loggerNamePrefix + i, messageFactory, - (name, factory) -> new MockLogger(loggerContext, name, factory)); + (name, factory) -> new Logger(loggerContext, name, factory) {}); logger.info("Using logger {}", logger.getName()); } await().atMost(10, SECONDS).pollInterval(100, MILLISECONDS).untilAsserted(() -> { System.gc(); - System.runFinalization(); registry.computeIfAbsent( - "triggerExpunge", messageFactory, (name, factory) -> new MockLogger(loggerContext, name, factory)); + "triggerExpunge", messageFactory, (name, factory) -> new Logger(loggerContext, name, factory) {}); - Map>> loggerRefByNameByMessageFactory = + final Map>> loggerRefByNameByMessageFactory = reflectAndGetLoggerMapFromRegistry(); - Map> loggerRefByName = loggerRefByNameByMessageFactory.get(messageFactory); + final Map> loggerRefByName = + loggerRefByNameByMessageFactory.get(messageFactory); - boolean isExpungeStaleEntries = true; + int unexpectedCount = 0; for (int i = 0; i < numberOfLoggers; i++) { if (loggerRefByName.containsKey(loggerNamePrefix + i)) { - isExpungeStaleEntries = false; - break; + unexpectedCount++; } } - assertTrue( - isExpungeStaleEntries, - "Stale WeakReference entries were not removed from the inner map for MessageFactory"); + assertEquals( + 0, unexpectedCount, "Found " + unexpectedCount + " unexpected stale entries for MessageFactory"); }); } @Test void testExpungeStaleMessageFactoryEntry() { - SimpleMessageFactory mockMessageFactory = new SimpleMessageFactory(); + final SimpleMessageFactory mockMessageFactory = new SimpleMessageFactory(); Logger logger = registry.computeIfAbsent( - "testLogger", mockMessageFactory, (name, factory) -> new MockLogger(loggerContext, name, factory)); + "testLogger", mockMessageFactory, (name, factory) -> new Logger(loggerContext, name, factory) {}); logger.info("Using logger {}", logger.getName()); logger = null; await().atMost(10, SECONDS).pollInterval(100, MILLISECONDS).untilAsserted(() -> { System.gc(); - System.runFinalization(); registry.getLogger("triggerExpunge", mockMessageFactory); - Map>> loggerRefByNameByMessageFactory = + final Map>> loggerRefByNameByMessageFactory = reflectAndGetLoggerMapFromRegistry(); assertNull( loggerRefByNameByMessageFactory.get(mockMessageFactory), @@ -147,43 +141,13 @@ void testExpungeStaleMessageFactoryEntry() { }); } - @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) -> new MockLogger(loggerContext, 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"); - } - private Map>> reflectAndGetLoggerMapFromRegistry() throws NoSuchFieldException, IllegalAccessException { - Field loggerMapField = registry.getClass().getDeclaredField("loggerRefByNameByMessageFactory"); + final Field loggerMapField = registry.getClass().getDeclaredField("loggerRefByNameByMessageFactory"); loggerMapField.setAccessible(true); @SuppressWarnings("unchecked") - Map>> loggerMap = + final Map>> loggerMap = (Map>>) loggerMapField.get(registry); return loggerMap; } - - private class MockLogger extends Logger { - - protected MockLogger(LoggerContext context, String name, MessageFactory messageFactory) { - super(context, name, messageFactory); - } - } } 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 index 6647285da97..d75c47e978e 100644 --- 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 @@ -70,7 +70,7 @@ public InternalLoggerRegistry() {} * Expunges stale entries for logger references and message factories. */ private void expungeStaleEntries() { - Reference loggerRef = staleLoggerRefs.poll(); + final Reference loggerRef = staleLoggerRefs.poll(); if (loggerRef != null) { writeLock.lock(); @@ -79,14 +79,17 @@ private void expungeStaleEntries() { // Clear refQueue } - Iterator>>> outerIt = - loggerRefByNameByMessageFactory.entrySet().iterator(); - while (outerIt.hasNext()) { - Map.Entry>> outerEntry = outerIt.next(); - Map> innerMap = outerEntry.getValue(); - innerMap.values().removeIf(weakRef -> weakRef.get() == null); - if (innerMap.isEmpty()) { - outerIt.remove(); + final Iterator>>> + loggerRefByNameByMessageFactoryEntryIt = + loggerRefByNameByMessageFactory.entrySet().iterator(); + while (loggerRefByNameByMessageFactoryEntryIt.hasNext()) { + final Map.Entry>> + loggerRefByNameByMessageFactoryEntry = loggerRefByNameByMessageFactoryEntryIt.next(); + final Map> loggerRefByName = + loggerRefByNameByMessageFactoryEntry.getValue(); + loggerRefByName.values().removeIf(weakRef -> weakRef.get() == null); + if (loggerRefByName.isEmpty()) { + loggerRefByNameByMessageFactoryEntryIt.remove(); } } } finally { @@ -105,7 +108,7 @@ private void expungeStaleEntries() { public @Nullable Logger getLogger(final String name, final MessageFactory messageFactory) { requireNonNull(name, "name"); requireNonNull(messageFactory, "messageFactory"); - expungeStaleEntries(); // Clean up before retrieving + expungeStaleEntries(); readLock.lock(); try { @@ -124,7 +127,7 @@ private void expungeStaleEntries() { } public Collection getLoggers() { - expungeStaleEntries(); // Clean up before retrieving + expungeStaleEntries(); readLock.lock(); try { @@ -166,7 +169,7 @@ public boolean hasLogger(final String name, final MessageFactory messageFactory) public boolean hasLogger(final String name, final Class messageFactoryClass) { requireNonNull(name, "name"); requireNonNull(messageFactoryClass, "messageFactoryClass"); - expungeStaleEntries(); // Clean up before checking + expungeStaleEntries(); readLock.lock(); try { @@ -187,8 +190,7 @@ public Logger computeIfAbsent( requireNonNull(name, "name"); requireNonNull(messageFactory, "messageFactory"); requireNonNull(loggerSupplier, "loggerSupplier"); - - expungeStaleEntries(); // Clean up before adding a new logger + // Skipping `expungeStaleEntries()`, it will be invoked by the `getLogger()` invocation below // Read lock fast path: See if logger already exists @Nullable Logger logger = getLogger(name, messageFactory);