Skip to content

Commit 04facd7

Browse files
Suvrat1629jhl221123
Suvrat1629
authored and
jhl221123
committed
ensured stale loggers are removed from InternalLoggerRegistry and also added InternalLoggerRegistryGCTest
1 parent 9176b44 commit 04facd7

File tree

2 files changed

+162
-49
lines changed

2 files changed

+162
-49
lines changed

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

Lines changed: 122 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,122 @@
1+
/*
2+
* Licensed to the Apache Software Foundation (ASF) under one or more
3+
* contributor license agreements. See the NOTICE file distributed with
4+
* this work for additional information regarding copyright ownership.
5+
* The ASF licenses this file to you under the Apache License, Version 2.0
6+
* (the "License"); you may not use this file except in compliance with
7+
* the License. You may obtain a copy of the License at
8+
*
9+
* http://www.apache.org/licenses/LICENSE-2.0
10+
*
11+
* Unless required by applicable law or agreed to in writing, software
12+
* distributed under the License is distributed on an "AS IS" BASIS,
13+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
14+
* See the License for the specific language governing permissions and
15+
* limitations under the License.
16+
*/
17+
package org.apache.logging.log4j.core.test.util;
18+
19+
import static org.junit.jupiter.api.Assertions.*;
20+
21+
import java.lang.ref.WeakReference;
22+
import java.util.concurrent.CountDownLatch;
23+
import java.util.concurrent.ExecutorService;
24+
import java.util.concurrent.Executors;
25+
import org.apache.logging.log4j.core.Logger;
26+
import org.apache.logging.log4j.core.LoggerContext;
27+
import org.apache.logging.log4j.core.util.internal.InternalLoggerRegistry;
28+
import org.apache.logging.log4j.message.MessageFactory;
29+
import org.apache.logging.log4j.message.SimpleMessageFactory;
30+
import org.junit.jupiter.api.BeforeEach;
31+
import org.junit.jupiter.api.Test;
32+
33+
class InternalLoggerRegistryTest {
34+
private InternalLoggerRegistry registry;
35+
private MessageFactory messageFactory;
36+
37+
@BeforeEach
38+
void setUp() {
39+
registry = new InternalLoggerRegistry();
40+
messageFactory = new SimpleMessageFactory();
41+
}
42+
43+
@Test
44+
void testGetLoggerReturnsNullForNonExistentLogger() {
45+
assertNull(registry.getLogger("nonExistent", messageFactory));
46+
}
47+
48+
@Test
49+
void testComputeIfAbsentCreatesLogger() {
50+
Logger logger =
51+
registry.computeIfAbsent("testLogger", messageFactory, (name, factory) -> LoggerContext.getContext()
52+
.getLogger(name, factory));
53+
assertNotNull(logger);
54+
assertEquals("testLogger", logger.getName());
55+
}
56+
57+
@Test
58+
void testGetLoggerRetrievesExistingLogger() {
59+
Logger logger =
60+
registry.computeIfAbsent("testLogger", messageFactory, (name, factory) -> LoggerContext.getContext()
61+
.getLogger(name, factory));
62+
assertSame(logger, registry.getLogger("testLogger", messageFactory));
63+
}
64+
65+
@Test
66+
void testHasLoggerReturnsCorrectStatus() {
67+
assertFalse(registry.hasLogger("testLogger", messageFactory));
68+
registry.computeIfAbsent("testLogger", messageFactory, (name, factory) -> LoggerContext.getContext()
69+
.getLogger(name, factory));
70+
assertTrue(registry.hasLogger("testLogger", messageFactory));
71+
}
72+
73+
@Test
74+
void testExpungeStaleEntriesRemovesGarbageCollectedLoggers() throws InterruptedException {
75+
Logger logger =
76+
registry.computeIfAbsent("testLogger", messageFactory, (name, factory) -> LoggerContext.getContext()
77+
.getLogger(name, factory));
78+
79+
WeakReference<Logger> weakRef = new WeakReference<>(logger);
80+
logger = null; // Dereference to allow GC
81+
82+
// Retry loop to give GC time to collect
83+
for (int i = 0; i < 10; i++) {
84+
System.gc();
85+
Thread.sleep(100);
86+
if (weakRef.get() == null) {
87+
break;
88+
}
89+
}
90+
91+
// Access the registry to potentially trigger cleanup
92+
registry.computeIfAbsent("tempLogger", messageFactory, (name, factory) -> LoggerContext.getContext()
93+
.getLogger(name, factory));
94+
95+
assertNull(weakRef.get(), "Logger should have been garbage collected");
96+
assertNull(
97+
registry.getLogger("testLogger", messageFactory), "Stale logger should be removed from the registry");
98+
}
99+
100+
@Test
101+
void testConcurrentAccess() throws InterruptedException {
102+
int threadCount = 10;
103+
ExecutorService executor = Executors.newFixedThreadPool(threadCount);
104+
CountDownLatch latch = new CountDownLatch(threadCount);
105+
106+
for (int i = 0; i < threadCount; i++) {
107+
executor.submit(() -> {
108+
registry.computeIfAbsent("testLogger", messageFactory, (name, factory) -> LoggerContext.getContext()
109+
.getLogger(name, factory));
110+
latch.countDown();
111+
});
112+
}
113+
114+
latch.await();
115+
executor.shutdown();
116+
117+
// Verify logger was created and is accessible after concurrent creation
118+
assertNotNull(
119+
registry.getLogger("testLogger", messageFactory),
120+
"Logger should be accessible after concurrent creation");
121+
}
122+
}

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

Lines changed: 40 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,8 @@
1818

1919
import static java.util.Objects.requireNonNull;
2020

21+
import java.lang.ref.Reference;
22+
import java.lang.ref.ReferenceQueue;
2123
import java.lang.ref.WeakReference;
2224
import java.util.Collection;
2325
import java.util.HashMap;
@@ -39,10 +41,8 @@
3941
/**
4042
* A registry of {@link Logger}s namespaced by name and message factory.
4143
* This class is internally used by {@link LoggerContext}.
42-
* <p>
43-
* 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.
44-
* This also allows Log4j Core to evolve independently from Log4j API.
45-
* </p>
44+
*
45+
* Handles automatic cleanup of stale logger references to prevent memory leaks.
4646
*
4747
* @since 2.25.0
4848
*/
@@ -53,23 +53,44 @@ public final class InternalLoggerRegistry {
5353
new WeakHashMap<>();
5454

5555
private final ReadWriteLock lock = new ReentrantReadWriteLock();
56-
5756
private final Lock readLock = lock.readLock();
58-
5957
private final Lock writeLock = lock.writeLock();
6058

59+
// ReferenceQueue to track stale WeakReferences
60+
private final ReferenceQueue<Logger> staleLoggerRefs = new ReferenceQueue<>();
61+
6162
public InternalLoggerRegistry() {}
6263

64+
/**
65+
* Expunges stale logger references from the registry.
66+
*/
67+
private void expungeStaleEntries() {
68+
Reference<? extends Logger> loggerRef;
69+
while ((loggerRef = staleLoggerRefs.poll()) != null) {
70+
removeLogger(loggerRef);
71+
}
72+
}
73+
74+
/**
75+
* Removes a logger from the registry.
76+
*/
77+
private void removeLogger(Reference<? extends Logger> loggerRef) {
78+
writeLock.lock();
79+
try {
80+
loggerRefByNameByMessageFactory.values().forEach(map -> map.values().removeIf(ref -> ref == loggerRef));
81+
} finally {
82+
writeLock.unlock();
83+
}
84+
}
85+
6386
/**
6487
* Returns the logger associated with the given name and message factory.
65-
*
66-
* @param name a logger name
67-
* @param messageFactory a message factory
68-
* @return the logger associated with the given name and message factory
6988
*/
7089
public @Nullable Logger getLogger(final String name, final MessageFactory messageFactory) {
7190
requireNonNull(name, "name");
7291
requireNonNull(messageFactory, "messageFactory");
92+
expungeStaleEntries(); // Clean up before retrieving
93+
7394
readLock.lock();
7495
try {
7596
final Map<String, WeakReference<Logger>> loggerRefByName =
@@ -87,11 +108,10 @@ public InternalLoggerRegistry() {}
87108
}
88109

89110
public Collection<Logger> getLoggers() {
111+
expungeStaleEntries(); // Clean up before retrieving
112+
90113
readLock.lock();
91114
try {
92-
// Return a new collection to allow concurrent iteration over the loggers
93-
//
94-
// https://github.com/apache/logging-log4j2/issues/3234
95115
return loggerRefByNameByMessageFactory.values().stream()
96116
.flatMap(loggerRefByName -> loggerRefByName.values().stream())
97117
.flatMap(loggerRef -> {
@@ -104,29 +124,17 @@ public Collection<Logger> getLoggers() {
104124
}
105125
}
106126

107-
/**
108-
* Checks if a logger associated with the given name and message factory exists.
109-
*
110-
* @param name a logger name
111-
* @param messageFactory a message factory
112-
* @return {@code true}, if the logger exists; {@code false} otherwise.
113-
*/
114127
public boolean hasLogger(final String name, final MessageFactory messageFactory) {
115128
requireNonNull(name, "name");
116129
requireNonNull(messageFactory, "messageFactory");
117130
return getLogger(name, messageFactory) != null;
118131
}
119132

120-
/**
121-
* Checks if a logger associated with the given name and message factory type exists.
122-
*
123-
* @param name a logger name
124-
* @param messageFactoryClass a message factory class
125-
* @return {@code true}, if the logger exists; {@code false} otherwise.
126-
*/
127133
public boolean hasLogger(final String name, final Class<? extends MessageFactory> messageFactoryClass) {
128134
requireNonNull(name, "name");
129135
requireNonNull(messageFactoryClass, "messageFactoryClass");
136+
expungeStaleEntries(); // Clean up before checking
137+
130138
readLock.lock();
131139
try {
132140
return loggerRefByNameByMessageFactory.entrySet().stream()
@@ -142,37 +150,21 @@ public Logger computeIfAbsent(
142150
final MessageFactory messageFactory,
143151
final BiFunction<String, MessageFactory, Logger> loggerSupplier) {
144152

145-
// Check arguments
146153
requireNonNull(name, "name");
147154
requireNonNull(messageFactory, "messageFactory");
148155
requireNonNull(loggerSupplier, "loggerSupplier");
149156

150-
// Read lock fast path: See if logger already exists
157+
expungeStaleEntries(); // Clean up before adding a new logger
158+
151159
@Nullable Logger logger = getLogger(name, messageFactory);
152160
if (logger != null) {
153161
return logger;
154162
}
155163

156-
// Intentionally moving the logger creation outside the write lock, because:
157-
//
158-
// - Logger instantiation is expensive (causes contention on the write-lock)
159-
//
160-
// - User code might have circular code paths, though through different threads.
161-
// Consider `T1[ILR:computeIfAbsent] -> ... -> T1[Logger::new] -> ... -> T2[ILR::computeIfAbsent]`.
162-
// Hence, having logger instantiation while holding a write lock might cause deadlocks:
163-
// https://github.com/apache/logging-log4j2/issues/3252
164-
// https://github.com/apache/logging-log4j2/issues/3399
165-
//
166-
// - Creating loggers without a lock, allows multiple threads to create loggers in parallel, which also improves
167-
// performance.
168-
//
169-
// Since all loggers with the same parameters are equivalent, we can safely return the logger from the
170-
// thread that finishes first.
171164
Logger newLogger = loggerSupplier.apply(name, messageFactory);
172-
173-
// Report name and message factory mismatch if there are any
174165
final String loggerName = newLogger.getName();
175166
final MessageFactory loggerMessageFactory = newLogger.getMessageFactory();
167+
176168
if (!loggerName.equals(name) || !loggerMessageFactory.equals(messageFactory)) {
177169
StatusLogger.getLogger()
178170
.error(
@@ -186,17 +178,16 @@ public Logger computeIfAbsent(
186178
messageFactory);
187179
}
188180

189-
// Write lock slow path: Insert the logger
190181
writeLock.lock();
191182
try {
192183
Map<String, WeakReference<Logger>> loggerRefByName = loggerRefByNameByMessageFactory.get(messageFactory);
193-
// noinspection Java8MapApi (avoid the allocation of lambda passed to `Map::computeIfAbsent`)
194184
if (loggerRefByName == null) {
195185
loggerRefByNameByMessageFactory.put(messageFactory, loggerRefByName = new HashMap<>());
196186
}
187+
197188
final WeakReference<Logger> loggerRef = loggerRefByName.get(name);
198189
if (loggerRef == null || (logger = loggerRef.get()) == null) {
199-
loggerRefByName.put(name, new WeakReference<>(logger = newLogger));
190+
loggerRefByName.put(name, new WeakReference<>(logger = newLogger, staleLoggerRefs));
200191
}
201192
return logger;
202193
} finally {

0 commit comments

Comments
 (0)