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..59f7bdd0b68 --- /dev/null +++ b/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 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..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 @@ -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}. *

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

* @@ -53,13 +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 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) { + Logger logger = loggerRef.get(); + if (logger == null) return; // Logger already cleared + + MessageFactory messageFactory = logger.getMessageFactory(); + String name = logger.getName(); + + writeLock.lock(); + try { + Map> loggerRefByName = loggerRefByNameByMessageFactory.get(messageFactory); + if (loggerRefByName != null) { + loggerRefByName.remove(name); + if (loggerRefByName.isEmpty()) { + loggerRefByNameByMessageFactory.remove(messageFactory); // Cleanup + } + } + } finally { + writeLock.unlock(); + } + } + /** * Returns the logger associated with the given name and message factory. * @@ -70,6 +109,8 @@ public InternalLoggerRegistry() {} 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,6 +128,8 @@ public InternalLoggerRegistry() {} } public Collection getLoggers() { + expungeStaleEntries(); // Clean up before retrieving + readLock.lock(); try { // Return a new collection to allow concurrent iteration over the loggers @@ -127,6 +170,8 @@ 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 + readLock.lock(); try { return loggerRefByNameByMessageFactory.entrySet().stream() @@ -147,6 +192,8 @@ public Logger computeIfAbsent( 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) { @@ -194,9 +241,10 @@ public Logger 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 {