diff --git a/log4j-api-test/pom.xml b/log4j-api-test/pom.xml index e1fb806b9b4..aef8a1b5d64 100644 --- a/log4j-api-test/pom.xml +++ b/log4j-api-test/pom.xml @@ -144,6 +144,11 @@ mockito-inline test + + org.jspecify + jspecify + test + org.osgi 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 > loggerRefByMessageFactory = - loggerRefByMessageFactoryByName.get(name); - if (loggerRefByMessageFactory == null) { - return null; - } + final @Nullable Map loggerByMessageFactory = loggerByMessageFactoryByName.get(name); final MessageFactory effectiveMessageFactory = messageFactory != null ? messageFactory : ParameterizedMessageFactory.INSTANCE; - final WeakReference loggerRef = loggerRefByMessageFactory.get(effectiveMessageFactory); - if (loggerRef == null) { - return null; - } - return loggerRef.get(); + return loggerByMessageFactory == null ? null : loggerByMessageFactory.get(effectiveMessageFactory); } 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) + loggerByMessageFactoryByName.values().stream() + .flatMap(loggerByMessageFactory -> loggerByMessageFactory.values().stream()) .forEach(destination::add); } finally { readLock.unlock(); @@ -196,7 +186,7 @@ public Collection getLoggers(final Collection destination) { */ public boolean hasLogger(final String name) { requireNonNull(name, "name"); - final T logger = getLogger(name); + final @Nullable T logger = getLogger(name); return logger != null; } @@ -215,7 +205,7 @@ public boolean hasLogger(final String name) { */ public boolean hasLogger(final String name, final MessageFactory messageFactory) { requireNonNull(name, "name"); - final T logger = getLogger(name, messageFactory); + final @Nullable T logger = getLogger(name, messageFactory); return logger != null; } @@ -232,7 +222,7 @@ public boolean hasLogger(final String name, final Class messageFactoryClass.equals(messageFactory.getClass())); } finally { readLock.unlock(); @@ -241,34 +231,42 @@ public boolean hasLogger(final String name, final ClassLogger name and message factory parameters are ignored, those will be obtained from the logger instead. + *

+ * The logger will be registered using the keys provided by the {@code name} and {@code messageFactory} parameters + * and the values of {@link Logger#getName()} and {@link Logger#getMessageFactory()}. + *

* - * @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(final String name, 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 Map> loggerRefByMessageFactory = - loggerRefByMessageFactoryByName.computeIfAbsent( - logger.getName(), 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)); + final MessageFactory effectiveMessageFactory = + messageFactory != null ? messageFactory : ParameterizedMessageFactory.INSTANCE; + // Register using the keys provided by the caller + loggerByMessageFactoryByName + .computeIfAbsent(name, this::createLoggerRefByMessageFactoryMap) + .putIfAbsent(effectiveMessageFactory, logger); + // Also register using the values extracted from `logger` + if (!name.equals(logger.getName()) || !effectiveMessageFactory.equals(logger.getMessageFactory())) { + loggerByMessageFactoryByName + .computeIfAbsent(logger.getName(), this::createLoggerRefByMessageFactoryMap) + .putIfAbsent(logger.getMessageFactory(), logger); } } finally { writeLock.unlock(); } } - private Map> createLoggerRefByMessageFactoryMap(final String ignored) { + private Map createLoggerRefByMessageFactoryMap(final String ignored) { return new WeakHashMap<>(); } } 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; 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 88f66ac364d..d084fd24843 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; @@ -82,7 +83,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; @@ -512,7 +513,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()); } /** @@ -526,13 +527,7 @@ public Collection getLoggers() { public Logger getLogger(final String name, final MessageFactory messageFactory) { final MessageFactory effectiveMessageFactory = messageFactory != null ? messageFactory : DEFAULT_MESSAGE_FACTORY; - final Logger oldLogger = loggerRegistry.getLogger(name, effectiveMessageFactory); - if (oldLogger != null) { - return oldLogger; - } - final Logger newLogger = newInstance(name, effectiveMessageFactory); - loggerRegistry.putIfAbsent(name, effectiveMessageFactory, newLogger); - return loggerRegistry.getLogger(name, effectiveMessageFactory); + return loggerRegistry.computeIfAbsent(name, effectiveMessageFactory, this::newInstance); } /** @@ -541,8 +536,11 @@ public Logger getLogger(final String name, final MessageFactory messageFactory) * @return the LoggerRegistry. * @since 2.17.2 */ - public LoggerRegistry getLoggerRegistry() { - return loggerRegistry; + 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; } /** @@ -775,9 +773,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..5aec88f599d --- /dev/null +++ b/log4j-core/src/main/java/org/apache/logging/log4j/core/util/internal/InternalLoggerRegistry.java @@ -0,0 +1,186 @@ +/* + * 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 name and message factory mismatch if there are any + final String loggerName = logger.getName(); + 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 name `{}` or 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.", + loggerName, + loggerMessageFactory, + name, + messageFactory); + // Register logger under alternative keys + loggerRefByNameByMessageFactory + .computeIfAbsent(loggerMessageFactory, ignored -> new HashMap<>()) + .putIfAbsent(loggerName, new WeakReference<>(logger)); + } + + // Insert the logger + loggerRefByName.put(name, new WeakReference<>(logger)); + return logger; + } finally { + writeLock.unlock(); + } + } +} 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..39691cf8c8c --- /dev/null +++ b/src/changelog/.2.x.x/3143_logger_registry.xml @@ -0,0 +1,10 @@ + + + + + Use hard references to `Logger`s in `LoggerRegistry`. + +