Fix ConcurrentModificationException in updateLoggers()

The `InternalLoggerRegistry` implementation introduced in version `2.24.2` did not return a copy of the registry, when `InternalLoggerRegistry.getLoggers()` was called. This could lead to a `ConcurrentModificationException` if a thread creates a new logger, while another thread calls `LoggerContext.updateLoggers()`.

Closes #3234

Author: ppkarwasz

log4j-api/src/main/java/org/apache/logging/log4j/spi/LoggerRegistry.java

@@ -18,7 +18,6 @@
 
 import static java.util.Objects.requireNonNull;
 
-import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.HashMap;
@@ -28,6 +27,7 @@
 import java.util.concurrent.locks.Lock;
 import java.util.concurrent.locks.ReadWriteLock;
 import java.util.concurrent.locks.ReentrantReadWriteLock;
+import java.util.stream.Collectors;
 import org.apache.logging.log4j.Logger;
 import org.apache.logging.log4j.message.MessageFactory;
 import org.apache.logging.log4j.message.ParameterizedMessageFactory;
@@ -158,19 +158,19 @@ public LoggerRegistry(final MapFactory<T> mapFactory) {
     }
 
     public Collection<T> getLoggers() {
-        return getLoggers(new ArrayList<>());
-    }
-
-    public Collection<T> getLoggers(final Collection<T> destination) {
-        requireNonNull(destination, "destination");
         readLock.lock();
         try {
-            loggerByMessageFactoryByName.values().stream()
+            return loggerByMessageFactoryByName.values().stream()
                     .flatMap(loggerByMessageFactory -> loggerByMessageFactory.values().stream())
-                    .forEach(destination::add);
+                    .collect(Collectors.toList());
         } finally {
             readLock.unlock();
         }
+    }
+
+    public Collection<T> getLoggers(final Collection<T> destination) {
+        requireNonNull(destination, "destination");
+        destination.addAll(getLoggers());
         return destination;
     }
 

log4j-core-test/src/test/java/org/apache/logging/log4j/core/LoggerContextTest.java

@@ -19,13 +19,23 @@
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.mockito.Mockito.mock;
 
+import java.util.Collection;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.Future;
+import java.util.stream.Collectors;
+import java.util.stream.IntStream;
 import org.apache.logging.log4j.message.MessageFactory;
 import org.apache.logging.log4j.message.MessageFactory2;
+import org.junit.jupiter.api.Assertions;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.TestInfo;
 
 class LoggerContextTest {
 
+    private static final int LOGGER_COUNT = 1024;
+    private static final int CONCURRENCY_LEVEL = 16;
+
     @Test
     void newInstance_should_honor_name_and_message_factory(final TestInfo testInfo) {
         final String testName = testInfo.getDisplayName();
@@ -37,4 +47,32 @@ void newInstance_should_honor_name_and_message_factory(final TestInfo testInfo)
             assertThat((MessageFactory) logger.getMessageFactory()).isSameAs(messageFactory);
         }
     }
+
+    @Test
+    void getLoggers_can_be_updated_concurrently(final TestInfo testInfo) {
+        final String testName = testInfo.getDisplayName();
+        final ExecutorService executorService = Executors.newFixedThreadPool(CONCURRENCY_LEVEL);
+        try (LoggerContext loggerContext = new LoggerContext(testName)) {
+            // Create a logger
+            Collection<Future<?>> tasks = IntStream.range(0, CONCURRENCY_LEVEL)
+                    .mapToObj(i -> executorService.submit(() -> {
+                        // Iterates over loggers
+                        loggerContext.updateLoggers();
+                        // Create some loggers
+                        for (int j = 0; j < LOGGER_COUNT; j++) {
+                            loggerContext.getLogger(testName + "-" + i + "-" + j);
+                        }
+                        // Iterate over loggers again
+                        loggerContext.updateLoggers();
+                    }))
+                    .collect(Collectors.toList());
+            Assertions.assertDoesNotThrow(() -> {
+                for (Future<?> task : tasks) {
+                    task.get();
+                }
+            });
+        } finally {
+            executorService.shutdown();
+        }
+    }
 }

log4j-core/src/main/java/org/apache/logging/log4j/core/LoggerContext.java

@@ -33,7 +33,6 @@
 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;
@@ -513,7 +512,7 @@ public Logger getLogger(final String name) {
      * @return a collection of the current loggers.
      */
     public Collection<Logger> getLoggers() {
-        return loggerRegistry.getLoggers().collect(Collectors.toList());
+        return loggerRegistry.getLoggers();
     }
 
     /**

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

@@ -19,6 +19,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;
@@ -27,6 +28,7 @@
 import java.util.concurrent.locks.ReadWriteLock;
 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;
@@ -78,15 +80,19 @@ public InternalLoggerRegistry() {}
         }
     }
 
-    public Stream<Logger> getLoggers() {
+    public Collection<Logger> getLoggers() {
         readLock.lock();
         try {
+            // Return a new collection to allow concurrent iteration over the loggers
+            //
+            // 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();
         }

src/changelog/.2.x.x/3234_concurrent-logger-modification.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="3234" link="https://github.com/apache/logging-log4j2/issues/3234"/>
+  <description format="asciidoc">
+    Fix `ConcurrentModificationException`, if multiple threads iterate over the loggers at the same time.
+  </description>
+</entry>