From b3081e1f198eac2397b857b25382cfb725dc34aa Mon Sep 17 00:00:00 2001 From: "Piotr P. Karwasz" Date: Mon, 11 Nov 2024 17:31:18 +0100 Subject: [PATCH 1/7] Removes weak references from `LoggerRepository` 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. --- .../logging/log4j/spi/LoggerRegistry.java | 70 +++++++------------ src/changelog/.2.x.x/3143_logger_registry.xml | 10 +++ 2 files changed, 35 insertions(+), 45 deletions(-) create mode 100644 src/changelog/.2.x.x/3143_logger_registry.xml diff --git a/log4j-api/src/main/java/org/apache/logging/log4j/spi/LoggerRegistry.java b/log4j-api/src/main/java/org/apache/logging/log4j/spi/LoggerRegistry.java index 51519a33d03..0e0fb6b8321 100644 --- a/log4j-api/src/main/java/org/apache/logging/log4j/spi/LoggerRegistry.java +++ b/log4j-api/src/main/java/org/apache/logging/log4j/spi/LoggerRegistry.java @@ -18,13 +18,11 @@ 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.Optional; import java.util.WeakHashMap; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.locks.Lock; @@ -43,7 +41,7 @@ @NullMarked public class LoggerRegistry { - private final Map>> loggerRefByMessageFactoryByName = new HashMap<>(); + private final Map> loggerByNameByMessageFactory = new WeakHashMap<>(); private final ReadWriteLock lock = new ReentrantReadWriteLock(); @@ -143,7 +141,7 @@ public LoggerRegistry(@Nullable final MapFactory mapFactory) { * Use {@link #getLogger(String, MessageFactory)} instead. */ @Deprecated - public T getLogger(final String name) { + public @Nullable T getLogger(final String name) { requireNonNull(name, "name"); return getLogger(name, null); } @@ -160,39 +158,31 @@ 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, @Nullable final MessageFactory messageFactory) { + public @Nullable T getLogger(final String name, @Nullable final MessageFactory messageFactory) { requireNonNull(name, "name"); readLock.lock(); try { - final Map> loggerRefByMessageFactory = - loggerRefByMessageFactoryByName.get(name); - if (loggerRefByMessageFactory == null) { - return null; - } final MessageFactory effectiveMessageFactory = messageFactory != null ? messageFactory : ParameterizedMessageFactory.INSTANCE; - final WeakReference loggerRef = loggerRefByMessageFactory.get(effectiveMessageFactory); - if (loggerRef == null) { - return null; - } - return loggerRef.get(); + return Optional.of(loggerByNameByMessageFactory) + .map(loggerByNameByMessageFactory -> loggerByNameByMessageFactory.get(effectiveMessageFactory)) + .map(loggerByName -> loggerByName.get(name)) + .orElse(null); } finally { readLock.unlock(); } } public Collection getLoggers() { - return getLoggers(new ArrayList()); + return getLoggers(new ArrayList<>()); } public Collection getLoggers(final Collection destination) { requireNonNull(destination, "destination"); readLock.lock(); try { - loggerRefByMessageFactoryByName.values().stream() - .flatMap(loggerRefByMessageFactory -> - loggerRefByMessageFactory.values().stream().map(WeakReference::get)) - .filter(Objects::nonNull) + loggerByNameByMessageFactory.values().stream() + .flatMap(loggerByName -> loggerByName.values().stream()) .forEach(destination::add); } finally { readLock.unlock(); @@ -215,8 +205,7 @@ public Collection getLoggers(final Collection destination) { @Deprecated public boolean hasLogger(final String name) { requireNonNull(name, "name"); - final T logger = getLogger(name); - return logger != null; + return getLogger(name) != null; } /** @@ -234,8 +223,7 @@ public boolean hasLogger(final String name) { */ public boolean hasLogger(final String name, @Nullable final MessageFactory messageFactory) { requireNonNull(name, "name"); - final T logger = getLogger(name, messageFactory); - return logger != null; + return getLogger(name, messageFactory) != null; } /** @@ -251,8 +239,9 @@ public boolean hasLogger(final String name, final Class messageFactoryClass.equals(messageFactory.getClass())); + return loggerByNameByMessageFactory.entrySet().stream() + .filter(entry -> messageFactoryClass.equals(entry.getKey().getClass())) + .anyMatch(entry -> entry.getValue().containsKey(name)); } finally { readLock.unlock(); } @@ -279,14 +268,10 @@ public void putIfAbsent( writeLock.lock(); try { final String loggerName = logger.getName(); - final Map> loggerRefByMessageFactory = - loggerRefByMessageFactoryByName.computeIfAbsent( - loggerName, this::createLoggerRefByMessageFactoryMap); final MessageFactory loggerMessageFactory = logger.getMessageFactory(); - final WeakReference loggerRef = loggerRefByMessageFactory.get(loggerMessageFactory); - if (loggerRef == null || loggerRef.get() == null) { - loggerRefByMessageFactory.put(loggerMessageFactory, new WeakReference<>(logger)); - } + loggerByNameByMessageFactory + .computeIfAbsent(loggerMessageFactory, ignored -> new HashMap<>()) + .putIfAbsent(loggerName, logger); } finally { writeLock.unlock(); } @@ -303,7 +288,7 @@ public T computeIfAbsent( requireNonNull(loggerSupplier, "loggerSupplier"); // Read lock fast path: See if logger already exists - T logger = getLogger(name, messageFactory); + @Nullable T logger = getLogger(name, messageFactory); if (logger != null) { return logger; } @@ -313,11 +298,10 @@ public T computeIfAbsent( try { // See if the logger is created by another thread in the meantime - final Map> loggerRefByMessageFactory = - loggerRefByMessageFactoryByName.computeIfAbsent(name, this::createLoggerRefByMessageFactoryMap); - final WeakReference loggerRef; - if ((loggerRef = loggerRefByMessageFactory.get(messageFactory)) != null - && (logger = loggerRef.get()) != null) { + final Map loggerByName = + loggerByNameByMessageFactory.computeIfAbsent(messageFactory, ignored -> new HashMap<>()); + logger = loggerByName.get(name); + if (logger != null) { return logger; } @@ -339,14 +323,10 @@ public T computeIfAbsent( } // Insert the logger - loggerRefByMessageFactory.put(loggerMessageFactory, new WeakReference<>(logger)); + loggerByName.put(name, logger); return logger; } finally { writeLock.unlock(); } } - - private Map> createLoggerRefByMessageFactoryMap(final String ignored) { - return new WeakHashMap<>(); - } } diff --git a/src/changelog/.2.x.x/3143_logger_registry.xml b/src/changelog/.2.x.x/3143_logger_registry.xml new file mode 100644 index 00000000000..d184e022a6a --- /dev/null +++ b/src/changelog/.2.x.x/3143_logger_registry.xml @@ -0,0 +1,10 @@ + + + + + Use hard references to `Logger`s in `LoggerRegistry`. + + From 51b7e7267105385f5dd4ad18ff5d1b77fd7e35a2 Mon Sep 17 00:00:00 2001 From: "Piotr P. Karwasz" Date: Wed, 13 Nov 2024 20:58:54 +0100 Subject: [PATCH 2/7] Create `InternalLoggerRegistry` in `log4j-core` --- .../logging/log4j/core/LoggerContext.java | 20 +- .../util/internal/InternalLoggerRegistry.java | 180 ++++++++++++++++++ 2 files changed, 192 insertions(+), 8 deletions(-) create mode 100644 log4j-core/src/main/java/org/apache/logging/log4j/core/util/internal/InternalLoggerRegistry.java diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/LoggerContext.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/LoggerContext.java index df8bb774819..0eac222c9e2 100644 --- a/log4j-core/src/main/java/org/apache/logging/log4j/core/LoggerContext.java +++ b/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; @@ -83,7 +84,7 @@ public class LoggerContext extends AbstractLifeCycle */ private static final MessageFactory DEFAULT_MESSAGE_FACTORY = Logger.getEffectiveMessageFactory(null); - private final LoggerRegistry loggerRegistry = new LoggerRegistry<>(); + private final InternalLoggerRegistry loggerRegistry = new InternalLoggerRegistry(); private final CopyOnWriteArrayList propertyChangeListeners = new CopyOnWriteArrayList<>(); private volatile List listeners; @@ -513,7 +514,7 @@ public Logger getLogger(final String name) { * @return a collection of the current loggers. */ public Collection getLoggers() { - return loggerRegistry.getLoggers(); + return loggerRegistry.getLoggers().collect(Collectors.toList()); } /** @@ -535,9 +536,14 @@ public Logger getLogger(final String name, @Nullable final MessageFactory messag * * @return the LoggerRegistry. * @since 2.17.2 + * @deprecated since 2.25.0 without a replacement. */ - public LoggerRegistry getLoggerRegistry() { - return loggerRegistry; + @Deprecated + public org.apache.logging.log4j.spi.LoggerRegistry getLoggerRegistry() { + org.apache.logging.log4j.spi.LoggerRegistry result = + new org.apache.logging.log4j.spi.LoggerRegistry<>(); + loggerRegistry.getLoggers().forEach(l -> result.putIfAbsent(l.getName(), l.getMessageFactory(), l)); + return result; } /** @@ -770,9 +776,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)); } 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 new file mode 100644 index 00000000000..8900d540c1c --- /dev/null +++ b/log4j-core/src/main/java/org/apache/logging/log4j/core/util/internal/InternalLoggerRegistry.java @@ -0,0 +1,180 @@ +/* + * 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.util.internal; + +import static java.util.Objects.requireNonNull; + +import java.lang.ref.WeakReference; +import java.util.HashMap; +import java.util.Map; +import java.util.Optional; +import java.util.WeakHashMap; +import java.util.concurrent.locks.Lock; +import java.util.concurrent.locks.ReadWriteLock; +import java.util.concurrent.locks.ReentrantReadWriteLock; +import java.util.function.BiFunction; +import java.util.stream.Stream; +import org.apache.logging.log4j.core.Logger; +import org.apache.logging.log4j.message.MessageFactory; +import org.apache.logging.log4j.status.StatusLogger; +import org.jspecify.annotations.NullMarked; +import org.jspecify.annotations.Nullable; + +/** + * Convenience class used by {@link org.apache.logging.log4j.core.LoggerContext} + *

+ * We don't use {@link org.apache.logging.log4j.spi.LoggerRegistry} from the Log4j API to keep Log4j Core independent + * from the version of the Log4j API at runtime. + *

+ * @since 2.25.0 + */ +@NullMarked +public final class InternalLoggerRegistry { + + private final Map>> loggerRefByNameByMessageFactory = + new WeakHashMap<>(); + + private final ReadWriteLock lock = new ReentrantReadWriteLock(); + + private final Lock readLock = lock.readLock(); + + private final Lock writeLock = lock.writeLock(); + + public InternalLoggerRegistry() {} + + /** + * 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"); + readLock.lock(); + try { + return Optional.of(loggerRefByNameByMessageFactory) + .map(loggerRefByNameByMessageFactory -> loggerRefByNameByMessageFactory.get(messageFactory)) + .map(loggerRefByName -> loggerRefByName.get(name)) + .map(WeakReference::get) + .orElse(null); + } finally { + readLock.unlock(); + } + } + + public Stream getLoggers() { + readLock.lock(); + try { + return loggerRefByNameByMessageFactory.values().stream() + .flatMap(loggerRefByName -> loggerRefByName.values().stream()) + .flatMap(loggerRef -> { + @Nullable Logger logger = loggerRef.get(); + return logger != null ? Stream.of(logger) : Stream.empty(); + }); + } finally { + readLock.unlock(); + } + } + + /** + * 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"); + readLock.lock(); + try { + return loggerRefByNameByMessageFactory.entrySet().stream() + .filter(entry -> messageFactoryClass.equals(entry.getKey().getClass())) + .anyMatch(entry -> entry.getValue().containsKey(name)); + } finally { + readLock.unlock(); + } + } + + public Logger computeIfAbsent( + final String name, + 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 + @Nullable Logger logger = getLogger(name, messageFactory); + if (logger != null) { + return logger; + } + + // Write lock slow path: Insert the logger + writeLock.lock(); + try { + + // See if the logger is created by another thread in the meantime + final Map> loggerRefByName = + loggerRefByNameByMessageFactory.computeIfAbsent(messageFactory, ignored -> new HashMap<>()); + WeakReference loggerRef = loggerRefByName.get(name); + if (loggerRef != null && (logger = loggerRef.get()) != null) { + return logger; + } + + // Create the logger + logger = loggerSupplier.apply(name, messageFactory); + + // Report message factory mismatches, if there is any + final MessageFactory loggerMessageFactory = logger.getMessageFactory(); + if (!loggerMessageFactory.equals(messageFactory)) { + StatusLogger.getLogger() + .error( + "Newly registered logger with name `{}` and message factory `{}`, is requested to be associated with a different message factory: `{}`.\n" + + "Effectively the message factory of the logger will be used and the other one will be ignored.\n" + + "This generally hints a problem at the logger context implementation.\n" + + "Please report this using the Log4j project issue tracker.", + name, + loggerMessageFactory, + messageFactory); + } + + // Insert the logger + loggerRefByName.put(name, new WeakReference<>(logger)); + return logger; + } finally { + writeLock.unlock(); + } + } +} From 780d88f9a9112fc15a47fbc9c5176a6fdc9d0e8b Mon Sep 17 00:00:00 2001 From: "Piotr P. Karwasz" Date: Wed, 13 Nov 2024 21:08:38 +0100 Subject: [PATCH 3/7] Add Javadoc to `computeIfAbsent` --- .../org/apache/logging/log4j/spi/LoggerRegistry.java | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/log4j-api/src/main/java/org/apache/logging/log4j/spi/LoggerRegistry.java b/log4j-api/src/main/java/org/apache/logging/log4j/spi/LoggerRegistry.java index 0e0fb6b8321..9abc2c4b6f4 100644 --- a/log4j-api/src/main/java/org/apache/logging/log4j/spi/LoggerRegistry.java +++ b/log4j-api/src/main/java/org/apache/logging/log4j/spi/LoggerRegistry.java @@ -257,7 +257,6 @@ public boolean hasLogger(final String name, final Class Date: Wed, 13 Nov 2024 22:56:45 +0100 Subject: [PATCH 4/7] Remove `computeIfAbsent` --- .../log4j/simple/SimpleLoggerContext.java | 7 +- .../logging/log4j/spi/LoggerRegistry.java | 66 ------------------- .../logging/log4j/spi/package-info.java | 2 +- .../taglib/Log4jTaglibLoggerContext.java | 7 +- .../logging/log4j/tojul/JULLoggerContext.java | 7 +- .../logging/slf4j/SLF4JLoggerContext.java | 7 +- 6 files changed, 25 insertions(+), 71 deletions(-) diff --git a/log4j-api/src/main/java/org/apache/logging/log4j/simple/SimpleLoggerContext.java b/log4j-api/src/main/java/org/apache/logging/log4j/simple/SimpleLoggerContext.java index 4663cbc9c6c..a0966bb07bb 100644 --- a/log4j-api/src/main/java/org/apache/logging/log4j/simple/SimpleLoggerContext.java +++ b/log4j-api/src/main/java/org/apache/logging/log4j/simple/SimpleLoggerContext.java @@ -102,7 +102,12 @@ public ExtendedLogger getLogger(final String name) { public ExtendedLogger getLogger(final String name, @Nullable final MessageFactory messageFactory) { final MessageFactory effectiveMessageFactory = messageFactory != null ? messageFactory : DEFAULT_MESSAGE_FACTORY; - return loggerRegistry.computeIfAbsent(name, effectiveMessageFactory, this::createLogger); + ExtendedLogger logger = loggerRegistry.getLogger(name, effectiveMessageFactory); + if (logger == null) { + logger = createLogger(name, effectiveMessageFactory); + loggerRegistry.putIfAbsent(name, effectiveMessageFactory, logger); + } + return logger; } private ExtendedLogger createLogger(final String name, @Nullable final MessageFactory messageFactory) { diff --git a/log4j-api/src/main/java/org/apache/logging/log4j/spi/LoggerRegistry.java b/log4j-api/src/main/java/org/apache/logging/log4j/spi/LoggerRegistry.java index 9abc2c4b6f4..9915bc26b12 100644 --- a/log4j-api/src/main/java/org/apache/logging/log4j/spi/LoggerRegistry.java +++ b/log4j-api/src/main/java/org/apache/logging/log4j/spi/LoggerRegistry.java @@ -28,10 +28,8 @@ import java.util.concurrent.locks.Lock; import java.util.concurrent.locks.ReadWriteLock; import java.util.concurrent.locks.ReentrantReadWriteLock; -import java.util.function.BiFunction; import org.apache.logging.log4j.message.MessageFactory; import org.apache.logging.log4j.message.ParameterizedMessageFactory; -import org.apache.logging.log4j.status.StatusLogger; import org.jspecify.annotations.NullMarked; import org.jspecify.annotations.Nullable; @@ -254,8 +252,6 @@ public boolean hasLogger(final String name, final Class loggerSupplier) { - - // Check arguments - requireNonNull(name, "name"); - requireNonNull(messageFactory, "messageFactory"); - requireNonNull(loggerSupplier, "loggerSupplier"); - - // Read lock fast path: See if logger already exists - @Nullable T logger = getLogger(name, messageFactory); - if (logger != null) { - return logger; - } - - // Write lock slow path: Insert the logger - writeLock.lock(); - try { - - // See if the logger is created by another thread in the meantime - final Map loggerByName = - loggerByNameByMessageFactory.computeIfAbsent(messageFactory, ignored -> new HashMap<>()); - logger = loggerByName.get(name); - if (logger != null) { - return logger; - } - - // Create the logger - logger = loggerSupplier.apply(name, messageFactory); - - // Report message factory mismatches, if there is any - final MessageFactory loggerMessageFactory = logger.getMessageFactory(); - if (!loggerMessageFactory.equals(messageFactory)) { - StatusLogger.getLogger() - .error( - "Newly registered logger with name `{}` and message factory `{}`, is requested to be associated with a different message factory: `{}`.\n" - + "Effectively the message factory of the logger will be used and the other one will be ignored.\n" - + "This generally hints a problem at the logger context implementation.\n" - + "Please report this using the Log4j project issue tracker.", - name, - loggerMessageFactory, - messageFactory); - } - - // Insert the logger - loggerByName.put(name, logger); - return logger; - } finally { - writeLock.unlock(); - } - } } diff --git a/log4j-api/src/main/java/org/apache/logging/log4j/spi/package-info.java b/log4j-api/src/main/java/org/apache/logging/log4j/spi/package-info.java index 3b6b5c25857..1748932083d 100644 --- a/log4j-api/src/main/java/org/apache/logging/log4j/spi/package-info.java +++ b/log4j-api/src/main/java/org/apache/logging/log4j/spi/package-info.java @@ -19,7 +19,7 @@ * API classes. */ @Export -@Version("2.25.0") +@Version("2.24.1") package org.apache.logging.log4j.spi; import org.osgi.annotation.bundle.Export; diff --git a/log4j-taglib/src/main/java/org/apache/logging/log4j/taglib/Log4jTaglibLoggerContext.java b/log4j-taglib/src/main/java/org/apache/logging/log4j/taglib/Log4jTaglibLoggerContext.java index 5d25ff6d89d..3a30a3ed12d 100644 --- a/log4j-taglib/src/main/java/org/apache/logging/log4j/taglib/Log4jTaglibLoggerContext.java +++ b/log4j-taglib/src/main/java/org/apache/logging/log4j/taglib/Log4jTaglibLoggerContext.java @@ -73,7 +73,12 @@ public Log4jTaglibLogger getLogger(final String name) { public Log4jTaglibLogger getLogger(final String name, @Nullable final MessageFactory messageFactory) { final MessageFactory effectiveMessageFactory = messageFactory != null ? messageFactory : DEFAULT_MESSAGE_FACTORY; - return loggerRegistry.computeIfAbsent(name, effectiveMessageFactory, this::createLogger); + Log4jTaglibLogger logger = loggerRegistry.getLogger(name, effectiveMessageFactory); + if (logger == null) { + logger = createLogger(name, effectiveMessageFactory); + loggerRegistry.putIfAbsent(name, effectiveMessageFactory, logger); + } + return logger; } private Log4jTaglibLogger createLogger(final String name, @Nullable final MessageFactory messageFactory) { diff --git a/log4j-to-jul/src/main/java/org/apache/logging/log4j/tojul/JULLoggerContext.java b/log4j-to-jul/src/main/java/org/apache/logging/log4j/tojul/JULLoggerContext.java index 9f92dca4931..675ea2cc900 100644 --- a/log4j-to-jul/src/main/java/org/apache/logging/log4j/tojul/JULLoggerContext.java +++ b/log4j-to-jul/src/main/java/org/apache/logging/log4j/tojul/JULLoggerContext.java @@ -52,7 +52,12 @@ public ExtendedLogger getLogger(final String name) { public ExtendedLogger getLogger(final String name, @Nullable final MessageFactory messageFactory) { final MessageFactory effectiveMessageFactory = messageFactory != null ? messageFactory : DEFAULT_MESSAGE_FACTORY; - return loggerRegistry.computeIfAbsent(name, effectiveMessageFactory, JULLoggerContext::createLogger); + ExtendedLogger logger = loggerRegistry.getLogger(name, effectiveMessageFactory); + if (logger == null) { + logger = createLogger(name, effectiveMessageFactory); + loggerRegistry.putIfAbsent(name, effectiveMessageFactory, logger); + } + return logger; } private static ExtendedLogger createLogger(final String name, @Nullable final MessageFactory messageFactory) { diff --git a/log4j-to-slf4j/src/main/java/org/apache/logging/slf4j/SLF4JLoggerContext.java b/log4j-to-slf4j/src/main/java/org/apache/logging/slf4j/SLF4JLoggerContext.java index ba2a1d48189..e14b0471491 100644 --- a/log4j-to-slf4j/src/main/java/org/apache/logging/slf4j/SLF4JLoggerContext.java +++ b/log4j-to-slf4j/src/main/java/org/apache/logging/slf4j/SLF4JLoggerContext.java @@ -45,7 +45,12 @@ public ExtendedLogger getLogger(final String name) { public ExtendedLogger getLogger(final String name, @Nullable final MessageFactory messageFactory) { final MessageFactory effectiveMessageFactory = messageFactory != null ? messageFactory : DEFAULT_MESSAGE_FACTORY; - return loggerRegistry.computeIfAbsent(name, effectiveMessageFactory, SLF4JLoggerContext::createLogger); + ExtendedLogger logger = loggerRegistry.getLogger(name, effectiveMessageFactory); + if (logger == null) { + logger = createLogger(name, effectiveMessageFactory); + loggerRegistry.putIfAbsent(name, effectiveMessageFactory, logger); + } + return logger; } private static ExtendedLogger createLogger(final String name, @Nullable final MessageFactory messageFactory) { From 9498a0dfaa5b7847651df4ff3f03334f07fa2352 Mon Sep 17 00:00:00 2001 From: "Piotr P. Karwasz" Date: Wed, 13 Nov 2024 22:59:57 +0100 Subject: [PATCH 5/7] Fix BND errors --- .../main/java/org/apache/logging/log4j/spi/package-info.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/log4j-api/src/main/java/org/apache/logging/log4j/spi/package-info.java b/log4j-api/src/main/java/org/apache/logging/log4j/spi/package-info.java index 1748932083d..1637d7a3d16 100644 --- a/log4j-api/src/main/java/org/apache/logging/log4j/spi/package-info.java +++ b/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; From 041e5ebdfb77fc0fc4e5ee136d0d9a73fe5f6919 Mon Sep 17 00:00:00 2001 From: "Piotr P. Karwasz" Date: Fri, 15 Nov 2024 13:12:55 +0100 Subject: [PATCH 6/7] Add Unit test --- .../logging/log4j/spi/LoggerRegistryTest.java | 64 +++++++++++++++++++ 1 file changed, 64 insertions(+) create mode 100644 log4j-api-test/src/test/java/org/apache/logging/log4j/spi/LoggerRegistryTest.java diff --git a/log4j-api-test/src/test/java/org/apache/logging/log4j/spi/LoggerRegistryTest.java b/log4j-api-test/src/test/java/org/apache/logging/log4j/spi/LoggerRegistryTest.java new file mode 100644 index 00000000000..269785ea714 --- /dev/null +++ b/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 Date: Fri, 15 Nov 2024 13:14:33 +0100 Subject: [PATCH 7/7] Minimize changes from `2.24.1` --- .../log4j/simple/SimpleLoggerContext.java | 11 +++-- .../logging/log4j/spi/LoggerRegistry.java | 47 ++++++++++--------- .../taglib/Log4jTaglibLoggerContext.java | 13 ++--- .../logging/log4j/tojul/JULLoggerContext.java | 11 +++-- .../logging/slf4j/SLF4JLoggerContext.java | 11 +++-- 5 files changed, 50 insertions(+), 43 deletions(-) diff --git a/log4j-api/src/main/java/org/apache/logging/log4j/simple/SimpleLoggerContext.java b/log4j-api/src/main/java/org/apache/logging/log4j/simple/SimpleLoggerContext.java index a0966bb07bb..55cced06f58 100644 --- a/log4j-api/src/main/java/org/apache/logging/log4j/simple/SimpleLoggerContext.java +++ b/log4j-api/src/main/java/org/apache/logging/log4j/simple/SimpleLoggerContext.java @@ -102,12 +102,13 @@ public ExtendedLogger getLogger(final String name) { public ExtendedLogger getLogger(final String name, @Nullable final MessageFactory messageFactory) { final MessageFactory effectiveMessageFactory = messageFactory != null ? messageFactory : DEFAULT_MESSAGE_FACTORY; - ExtendedLogger logger = loggerRegistry.getLogger(name, effectiveMessageFactory); - if (logger == null) { - logger = createLogger(name, effectiveMessageFactory); - loggerRegistry.putIfAbsent(name, effectiveMessageFactory, logger); + final ExtendedLogger oldLogger = loggerRegistry.getLogger(name, effectiveMessageFactory); + if (oldLogger != null) { + return oldLogger; } - return logger; + final ExtendedLogger newLogger = createLogger(name, effectiveMessageFactory); + loggerRegistry.putIfAbsent(name, effectiveMessageFactory, newLogger); + return loggerRegistry.getLogger(name, effectiveMessageFactory); } private ExtendedLogger createLogger(final String name, @Nullable final MessageFactory messageFactory) { diff --git a/log4j-api/src/main/java/org/apache/logging/log4j/spi/LoggerRegistry.java b/log4j-api/src/main/java/org/apache/logging/log4j/spi/LoggerRegistry.java index 9915bc26b12..24eb5417459 100644 --- a/log4j-api/src/main/java/org/apache/logging/log4j/spi/LoggerRegistry.java +++ b/log4j-api/src/main/java/org/apache/logging/log4j/spi/LoggerRegistry.java @@ -20,9 +20,9 @@ import java.util.ArrayList; import java.util.Collection; +import java.util.Collections; import java.util.HashMap; import java.util.Map; -import java.util.Optional; import java.util.WeakHashMap; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.locks.Lock; @@ -39,7 +39,7 @@ @NullMarked public class LoggerRegistry { - private final Map> loggerByNameByMessageFactory = new WeakHashMap<>(); + private final Map> loggerByMessageFactoryByName = new HashMap<>(); private final ReadWriteLock lock = new ReentrantReadWriteLock(); @@ -160,12 +160,10 @@ public LoggerRegistry(@Nullable final MapFactory mapFactory) { requireNonNull(name, "name"); readLock.lock(); try { + final @Nullable Map loggerByMessageFactory = loggerByMessageFactoryByName.get(name); final MessageFactory effectiveMessageFactory = messageFactory != null ? messageFactory : ParameterizedMessageFactory.INSTANCE; - return Optional.of(loggerByNameByMessageFactory) - .map(loggerByNameByMessageFactory -> loggerByNameByMessageFactory.get(effectiveMessageFactory)) - .map(loggerByName -> loggerByName.get(name)) - .orElse(null); + return loggerByMessageFactory == null ? null : loggerByMessageFactory.get(effectiveMessageFactory); } finally { readLock.unlock(); } @@ -179,8 +177,8 @@ public Collection getLoggers(final Collection destination) { requireNonNull(destination, "destination"); readLock.lock(); try { - loggerByNameByMessageFactory.values().stream() - .flatMap(loggerByName -> loggerByName.values().stream()) + loggerByMessageFactoryByName.values().stream() + .flatMap(loggerByMessageFactory -> loggerByMessageFactory.values().stream()) .forEach(destination::add); } finally { readLock.unlock(); @@ -203,7 +201,8 @@ public Collection getLoggers(final Collection destination) { @Deprecated public boolean hasLogger(final String name) { requireNonNull(name, "name"); - return getLogger(name) != null; + final @Nullable T logger = getLogger(name); + return logger != null; } /** @@ -221,7 +220,8 @@ public boolean hasLogger(final String name) { */ public boolean hasLogger(final String name, @Nullable final MessageFactory messageFactory) { requireNonNull(name, "name"); - return getLogger(name, messageFactory) != null; + final @Nullable T logger = getLogger(name, messageFactory); + return logger != null; } /** @@ -237,9 +237,8 @@ public boolean hasLogger(final String name, final Class messageFactoryClass.equals(entry.getKey().getClass())) - .anyMatch(entry -> entry.getValue().containsKey(name)); + return loggerByMessageFactoryByName.getOrDefault(name, Collections.emptyMap()).keySet().stream() + .anyMatch(messageFactory -> messageFactoryClass.equals(messageFactory.getClass())); } finally { readLock.unlock(); } @@ -249,26 +248,30 @@ public boolean hasLogger(final String name, final ClassLogger name and message factory parameters are ignored, those will be obtained from the logger instead. * - * @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( - @Nullable final String name, @Nullable 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 String loggerName = logger.getName(); - final MessageFactory loggerMessageFactory = logger.getMessageFactory(); - loggerByNameByMessageFactory - .computeIfAbsent(loggerMessageFactory, ignored -> new HashMap<>()) - .putIfAbsent(loggerName, logger); + final MessageFactory effectiveMessageFactory = + messageFactory != null ? messageFactory : ParameterizedMessageFactory.INSTANCE; + loggerByMessageFactoryByName + .computeIfAbsent(name, this::createLoggerRefByMessageFactoryMap) + .putIfAbsent(effectiveMessageFactory, logger); } finally { writeLock.unlock(); } } + + private Map createLoggerRefByMessageFactoryMap(final String ignored) { + return new WeakHashMap<>(); + } } diff --git a/log4j-taglib/src/main/java/org/apache/logging/log4j/taglib/Log4jTaglibLoggerContext.java b/log4j-taglib/src/main/java/org/apache/logging/log4j/taglib/Log4jTaglibLoggerContext.java index 3a30a3ed12d..97a7ab28bbe 100644 --- a/log4j-taglib/src/main/java/org/apache/logging/log4j/taglib/Log4jTaglibLoggerContext.java +++ b/log4j-taglib/src/main/java/org/apache/logging/log4j/taglib/Log4jTaglibLoggerContext.java @@ -73,15 +73,16 @@ public Log4jTaglibLogger getLogger(final String name) { public Log4jTaglibLogger getLogger(final String name, @Nullable final MessageFactory messageFactory) { final MessageFactory effectiveMessageFactory = messageFactory != null ? messageFactory : DEFAULT_MESSAGE_FACTORY; - Log4jTaglibLogger logger = loggerRegistry.getLogger(name, effectiveMessageFactory); - if (logger == null) { - logger = createLogger(name, effectiveMessageFactory); - loggerRegistry.putIfAbsent(name, effectiveMessageFactory, logger); + final Log4jTaglibLogger oldLogger = loggerRegistry.getLogger(name, effectiveMessageFactory); + if (oldLogger != null) { + return oldLogger; } - return logger; + final Log4jTaglibLogger newLogger = createLogger(name, effectiveMessageFactory); + loggerRegistry.putIfAbsent(name, effectiveMessageFactory, newLogger); + return loggerRegistry.getLogger(name, effectiveMessageFactory); } - private Log4jTaglibLogger createLogger(final String name, @Nullable final MessageFactory messageFactory) { + private Log4jTaglibLogger createLogger(final String name, final MessageFactory messageFactory) { final LoggerContext loggerContext = LogManager.getContext(false); final ExtendedLogger delegateLogger = loggerContext.getLogger(name, messageFactory); return new Log4jTaglibLogger(delegateLogger, name, delegateLogger.getMessageFactory()); diff --git a/log4j-to-jul/src/main/java/org/apache/logging/log4j/tojul/JULLoggerContext.java b/log4j-to-jul/src/main/java/org/apache/logging/log4j/tojul/JULLoggerContext.java index 675ea2cc900..6c18a83d485 100644 --- a/log4j-to-jul/src/main/java/org/apache/logging/log4j/tojul/JULLoggerContext.java +++ b/log4j-to-jul/src/main/java/org/apache/logging/log4j/tojul/JULLoggerContext.java @@ -52,12 +52,13 @@ public ExtendedLogger getLogger(final String name) { public ExtendedLogger getLogger(final String name, @Nullable final MessageFactory messageFactory) { final MessageFactory effectiveMessageFactory = messageFactory != null ? messageFactory : DEFAULT_MESSAGE_FACTORY; - ExtendedLogger logger = loggerRegistry.getLogger(name, effectiveMessageFactory); - if (logger == null) { - logger = createLogger(name, effectiveMessageFactory); - loggerRegistry.putIfAbsent(name, effectiveMessageFactory, logger); + final ExtendedLogger oldLogger = loggerRegistry.getLogger(name, effectiveMessageFactory); + if (oldLogger != null) { + return oldLogger; } - return logger; + final ExtendedLogger newLogger = createLogger(name, effectiveMessageFactory); + loggerRegistry.putIfAbsent(name, effectiveMessageFactory, newLogger); + return loggerRegistry.getLogger(name, effectiveMessageFactory); } private static ExtendedLogger createLogger(final String name, @Nullable final MessageFactory messageFactory) { diff --git a/log4j-to-slf4j/src/main/java/org/apache/logging/slf4j/SLF4JLoggerContext.java b/log4j-to-slf4j/src/main/java/org/apache/logging/slf4j/SLF4JLoggerContext.java index e14b0471491..50160e9063c 100644 --- a/log4j-to-slf4j/src/main/java/org/apache/logging/slf4j/SLF4JLoggerContext.java +++ b/log4j-to-slf4j/src/main/java/org/apache/logging/slf4j/SLF4JLoggerContext.java @@ -45,12 +45,13 @@ public ExtendedLogger getLogger(final String name) { public ExtendedLogger getLogger(final String name, @Nullable final MessageFactory messageFactory) { final MessageFactory effectiveMessageFactory = messageFactory != null ? messageFactory : DEFAULT_MESSAGE_FACTORY; - ExtendedLogger logger = loggerRegistry.getLogger(name, effectiveMessageFactory); - if (logger == null) { - logger = createLogger(name, effectiveMessageFactory); - loggerRegistry.putIfAbsent(name, effectiveMessageFactory, logger); + final ExtendedLogger oldLogger = loggerRegistry.getLogger(name, effectiveMessageFactory); + if (oldLogger != null) { + return oldLogger; } - return logger; + final ExtendedLogger newLogger = createLogger(name, effectiveMessageFactory); + loggerRegistry.putIfAbsent(name, effectiveMessageFactory, newLogger); + return loggerRegistry.getLogger(name, effectiveMessageFactory); } private static ExtendedLogger createLogger(final String name, @Nullable final MessageFactory messageFactory) {