3399/create logger no lock

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

@@ -18,9 +18,7 @@
 
 import static java.util.Objects.requireNonNull;
 
-import java.lang.ref.WeakReference;
 import java.util.Collection;
-import java.util.HashMap;
 import java.util.Map;
 import java.util.Optional;
 import java.util.WeakHashMap;
@@ -29,7 +27,6 @@
 import java.util.concurrent.locks.ReentrantReadWriteLock;
 import java.util.function.BiFunction;
 import java.util.stream.Collectors;
-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;
@@ -40,15 +37,14 @@
  * Convenience class used by {@link org.apache.logging.log4j.core.LoggerContext}
  * <p>
  *   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.
+ *   of the version of the Log4j API at runtime.
  * </p>
  * @since 2.25.0
  */
 @NullMarked
 public final class InternalLoggerRegistry {
 
-    private final Map<MessageFactory, Map<String, WeakReference<Logger>>> loggerRefByNameByMessageFactory =
-            new WeakHashMap<>();
+    private final Map<MessageFactory, Map<String, Logger>> loggerRefByNameByMessageFactory = new WeakHashMap<>();
 
     private final ReadWriteLock lock = new ReentrantReadWriteLock();
 
@@ -73,7 +69,6 @@ public InternalLoggerRegistry() {}
             return Optional.of(loggerRefByNameByMessageFactory)
                     .map(loggerRefByNameByMessageFactory -> loggerRefByNameByMessageFactory.get(messageFactory))
                     .map(loggerRefByName -> loggerRefByName.get(name))
-                    .map(WeakReference::get)
                     .orElse(null);
         } finally {
             readLock.unlock();
@@ -88,10 +83,6 @@ public Collection<Logger> getLoggers() {
             // https://github.com/apache/logging-log4j2/issues/3234
             return loggerRefByNameByMessageFactory.values().stream()
                     .flatMap(loggerRefByName -> loggerRefByName.values().stream())
-                    .flatMap(loggerRef -> {
-                        @Nullable Logger logger = loggerRef.get();
-                        return logger != null ? Stream.of(logger) : Stream.empty();
-                    })
                     .collect(Collectors.toList());
         } finally {
             readLock.unlock();
@@ -147,44 +138,38 @@ public Logger computeIfAbsent(
             return logger;
         }
 
+        // Creating a logger is expensive and might cause lookups and locks, possibly 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(
+                            "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);
+        }
+
         // Write lock slow path: Insert the logger
         writeLock.lock();
         try {
-
-            // See if the logger is created by another thread in the meantime
-            final Map<String, WeakReference<Logger>> loggerRefByName =
-                    loggerRefByNameByMessageFactory.computeIfAbsent(messageFactory, ignored -> new HashMap<>());
-            WeakReference<Logger> 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;
+            return loggerRefByNameByMessageFactory
+                    .computeIfAbsent(messageFactory, ignored -> new WeakValuesHashMap<>())
+                    .computeIfAbsent(name, ignored -> newLogger);
         } finally {
             writeLock.unlock();
         }

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

@@ -0,0 +1,121 @@
+/*
+ * 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 java.lang.ref.ReferenceQueue;
+import java.lang.ref.WeakReference;
+import java.util.AbstractMap;
+import java.util.Collection;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Set;
+
+/**
+ * A {@link HashMap} implementation which is similar to {@link java.util.WeakHashMap} but for weak values instead of
+ * keys. Warning: this is not a general-purpose implementation; only what's needed by @{@link InternalLoggerRegistry}
+ * is implemented.
+ * @param <K> - the type of keys maintained by this map
+ * @param <V> – the type of mapped values
+ */
+class WeakValuesHashMap<K, V> extends AbstractMap<K, V> {
+    private final HashMap<K, WeakValue<V>> map;
+    private final ReferenceQueue<V> queue;
+
+    public WeakValuesHashMap() {
+        map = new HashMap<>(10);
+        queue = new ReferenceQueue<>();
+    }
+
+    @Override
+    public V put(K key, V value) {
+        processQueue();
+        WeakValue<V> ref = new WeakValue<>(key, value, queue);
+        return unref(map.put(key, ref));
+    }
+
+    @Override
+    public V get(Object key) {
+        processQueue();
+        return unref(map.get(key));
+    }
+
+    @Override
+    public V remove(Object key) {
+        return unref(map.get(key));
+    }
+
+    @Override
+    public void clear() {
+        map.clear();
+    }
+
+    @Override
+    public boolean containsKey(Object key) {
+        processQueue();
+        return map.containsKey(key);
+    }
+
+    @Override
+    public boolean containsValue(Object value) {
+        throw new UnsupportedOperationException();
+    }
+
+    @Override
+    public Set<K> keySet() {
+        throw new UnsupportedOperationException();
+    }
+
+    @Override
+    public int size() {
+        processQueue();
+        return map.size();
+    }
+
+    @Override
+    public Set<Map.Entry<K, V>> entrySet() {
+        throw new UnsupportedOperationException();
+    }
+
+    @Override
+    public Collection<V> values() {
+        throw new UnsupportedOperationException();
+    }
+
+    private V unref(WeakValue<V> valueRef) {
+        return valueRef == null ? null : valueRef.get();
+    }
+
+    @SuppressWarnings("unchecked")
+    private void processQueue() {
+        for (WeakValue<V> v = (WeakValue<V>) queue.poll(); v != null; v = (WeakValue<V>) queue.poll()) {
+            map.remove(v.getKey());
+        }
+    }
+
+    private class WeakValue<T> extends WeakReference<T> {
+        private final K key;
+
+        private WeakValue(K key, T value, ReferenceQueue<T> queue) {
+            super(value, queue);
+            this.key = key;
+        }
+
+        private K getKey() {
+            return key;
+        }
+    }
+}

src/changelog/.2.x.x/3399_logger_registry.xml

@@ -0,0 +1,10 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<entry xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+       xmlns="https://logging.apache.org/xml/ns"
+       xsi:schemaLocation="https://logging.apache.org/xml/ns https://logging.apache.org/xml/ns/log4j-changelog-0.xsd"
+       type="fixed">
+  <issue id="3399" link="https://github.com/apache/logging-log4j2/issues/3399"/>
+  <description format="asciidoc">
+    Minimize lock usage in `InternalLoggerRegistry`.
+  </description>
+</entry>