Apply suggestions from review of #2374

log4j-api-test/src/main/java/org/apache/logging/log4j/test/TestLogger.java

@@ -75,18 +75,18 @@ protected void log(
         sb.append(level.toString());
         sb.append(' ');
         if (location != null) {
-            sb.append(location.toString());
+            sb.append(location);
             sb.append(' ');
         }
         sb.append(message.getFormattedMessage());
         final Map<String, String> mdc = ThreadContext.getImmutableContext();
-        if (mdc.size() > 0) {
+        if (!mdc.isEmpty()) {
             sb.append(' ');
-            sb.append(mdc.toString());
+            sb.append(mdc);
             sb.append(' ');
         }
         final Object[] params = message.getParameters();
-        Throwable t;
+        final Throwable t;
         if (throwable == null
                 && params != null
                 && params.length > 0
@@ -99,7 +99,7 @@ protected void log(
             sb.append(' ');
             final ByteArrayOutputStream baos = new ByteArrayOutputStream();
             t.printStackTrace(new PrintStream(baos));
-            sb.append(baos.toString());
+            sb.append(baos);
         }
         list.add(sb.toString());
         // System.out.println(sb.toString());

log4j-api-test/src/test/java/org/apache/logging/log4j/TestProvider.java

@@ -24,6 +24,6 @@
  */
 public class TestProvider extends Provider {
     public TestProvider() {
-        super(10, "2.6.0", TestLoggerContextFactory.class);
+        super(5, CURRENT_VERSION, TestLoggerContextFactory.class);
     }
 }

log4j-api-test/src/test/java/org/apache/logging/log4j/spi/ThreadContextMapTest.java

@@ -29,6 +29,8 @@
 
 class ThreadContextMapTest {
 
+    private static final String KEY = "key";
+
     static Stream<ThreadContextMap> defaultMaps() {
         return Stream.of(
                 new DefaultThreadContextMap(),
@@ -49,26 +51,24 @@ static Stream<ThreadContextMap> inheritableMaps() {
     @ParameterizedTest
     @MethodSource("defaultMaps")
     void threadLocalNotInheritableByDefault(final ThreadContextMap contextMap) {
-        contextMap.put("key", "threadLocalNotInheritableByDefault");
-        final ExecutorService executorService = Executors.newSingleThreadExecutor();
-        try {
-            assertThat(executorService.submit(() -> contextMap.get("key")))
-                    .succeedsWithin(Duration.ofSeconds(1))
-                    .isEqualTo(null);
-        } finally {
-            executorService.shutdown();
-        }
+        contextMap.put(KEY, "threadLocalNotInheritableByDefault");
+        verifyThreadContextValueFromANewThread(contextMap, null);
     }
 
     @ParameterizedTest
     @MethodSource("inheritableMaps")
     void threadLocalInheritableIfConfigured(final ThreadContextMap contextMap) {
-        contextMap.put("key", "threadLocalInheritableIfConfigured");
+        contextMap.put(KEY, "threadLocalInheritableIfConfigured");
+        verifyThreadContextValueFromANewThread(contextMap, "threadLocalInheritableIfConfigured");
+    }
+
+    private static void verifyThreadContextValueFromANewThread(
+            final ThreadContextMap contextMap, final String expected) {
         final ExecutorService executorService = Executors.newSingleThreadExecutor();
         try {
-            assertThat(executorService.submit(() -> contextMap.get("key")))
+            assertThat(executorService.submit(() -> contextMap.get(KEY)))
                     .succeedsWithin(Duration.ofSeconds(1))
-                    .isEqualTo("threadLocalInheritableIfConfigured");
+                    .isEqualTo(expected);
         } finally {
             executorService.shutdown();
         }

log4j-api-test/src/test/java/org/apache/logging/log4j/util/ProviderUtilTest.java

@@ -18,49 +18,141 @@
 
 import static org.assertj.core.api.Assertions.assertThat;
 
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.Collections;
 import java.util.Properties;
+import java.util.regex.Pattern;
+import java.util.stream.Stream;
 import org.apache.logging.log4j.TestProvider;
+import org.apache.logging.log4j.spi.Provider;
+import org.apache.logging.log4j.test.TestLogger;
 import org.apache.logging.log4j.test.TestLoggerContextFactory;
+import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.parallel.Execution;
 import org.junit.jupiter.api.parallel.ExecutionMode;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
 
 @Execution(ExecutionMode.CONCURRENT)
 class ProviderUtilTest {
 
-    /*
-     * Force initialization of ProviderUtil#PROVIDERS
-     */
-    static {
-        ProviderUtil.lazyInit();
+    private static final Pattern ERROR_OR_WARNING = Pattern.compile(" (ERROR|WARN) .*", Pattern.DOTALL);
+
+    private static final Provider LOCAL_PROVIDER = new LocalProvider();
+    private static final Provider TEST_PROVIDER = new TestProvider();
+
+    private static final Collection<Provider> NO_PROVIDERS = Collections.emptyList();
+    private static final Collection<Provider> ONE_PROVIDER = Collections.singleton(TEST_PROVIDER);
+    private static final Collection<Provider> TWO_PROVIDERS = Arrays.asList(LOCAL_PROVIDER, TEST_PROVIDER);
+
+    private TestLogger statusLogger;
+
+    @BeforeEach
+    void setup() {
+        statusLogger = new TestLogger();
+    }
+
+    @Test
+    void should_have_a_fallback_provider() {
+        final PropertiesUtil properties = new PropertiesUtil(new Properties());
+        assertThat(ProviderUtil.selectProvider(properties, NO_PROVIDERS, statusLogger))
+                .as("check selected provider")
+                .isNotNull();
+        // An error for the absence of providers
+        assertHasErrorOrWarning(statusLogger);
+    }
+
+    @Test
+    void should_be_silent_with_a_single_provider() {
+        final PropertiesUtil properties = new PropertiesUtil(new Properties());
+        assertThat(ProviderUtil.selectProvider(properties, ONE_PROVIDER, statusLogger))
+                .as("check selected provider")
+                .isSameAs(TEST_PROVIDER);
+        assertNoErrorsOrWarnings(statusLogger);
     }
 
     @Test
     void should_select_provider_with_highest_priority() {
         final PropertiesUtil properties = new PropertiesUtil(new Properties());
-        assertThat(ProviderUtil.selectProvider(properties))
+        assertThat(ProviderUtil.selectProvider(properties, TWO_PROVIDERS, statusLogger))
                 .as("check selected provider")
-                .isInstanceOf(TestProvider.class);
+                .isSameAs(TEST_PROVIDER);
+        // A warning for the presence of multiple providers
+        assertHasErrorOrWarning(statusLogger);
     }
 
     @Test
     void should_recognize_log4j_provider_property() {
         final Properties map = new Properties();
         map.setProperty("log4j.provider", LocalProvider.class.getName());
         final PropertiesUtil properties = new PropertiesUtil(map);
-        assertThat(ProviderUtil.selectProvider(properties))
+        assertThat(ProviderUtil.selectProvider(properties, TWO_PROVIDERS, statusLogger))
                 .as("check selected provider")
                 .isInstanceOf(LocalProvider.class);
+        assertNoErrorsOrWarnings(statusLogger);
     }
 
+    /**
+     * Can be removed in the future.
+     */
     @Test
     void should_recognize_log4j_factory_property() {
         final Properties map = new Properties();
         map.setProperty("log4j2.loggerContextFactory", LocalLoggerContextFactory.class.getName());
         final PropertiesUtil properties = new PropertiesUtil(map);
-        assertThat(ProviderUtil.selectProvider(properties).getLoggerContextFactory())
+        assertThat(ProviderUtil.selectProvider(properties, TWO_PROVIDERS, statusLogger)
+                        .getLoggerContextFactory())
                 .as("check selected logger context factory")
                 .isInstanceOf(LocalLoggerContextFactory.class);
+        // Deprecation warning
+        assertHasErrorOrWarning(statusLogger);
+    }
+
+    /**
+     * Can be removed in the future.
+     */
+    @Test
+    void log4j_provider_property_has_priority() {
+        final Properties map = new Properties();
+        map.setProperty("log4j.provider", LocalProvider.class.getName());
+        map.setProperty("log4j2.loggerContextFactory", TestLoggerContextFactory.class.getName());
+        final PropertiesUtil properties = new PropertiesUtil(map);
+        assertThat(ProviderUtil.selectProvider(properties, TWO_PROVIDERS, statusLogger))
+                .as("check selected provider")
+                .isInstanceOf(LocalProvider.class);
+        // Warning
+        assertHasErrorOrWarning(statusLogger);
+    }
+
+    static Stream<Arguments> incorrect_configuration_do_not_throw() {
+        return Stream.of(
+                Arguments.of("java.lang.String", null),
+                Arguments.of("non.existent.Provider", null),
+                // logger context factory without a matching provider
+                Arguments.of(null, "org.apache.logging.log4j.util.ProviderUtilTest$LocalLoggerContextFactory"),
+                Arguments.of(null, "java.lang.String"),
+                Arguments.of(null, "non.existent.LoggerContextFactory"));
+    }
+
+    @ParameterizedTest
+    @MethodSource
+    void incorrect_configuration_do_not_throw(final String provider, final String contextFactory) {
+        final Properties map = new Properties();
+        if (provider != null) {
+            map.setProperty("log4j.provider", provider);
+        }
+        if (contextFactory != null) {
+            map.setProperty("log4j2.loggerContextFactory", contextFactory);
+        }
+        final PropertiesUtil properties = new PropertiesUtil(map);
+        assertThat(ProviderUtil.selectProvider(properties, ONE_PROVIDER, statusLogger))
+                .as("check selected provider")
+                .isNotNull();
+        // Warnings will be present
+        assertHasErrorOrWarning(statusLogger);
     }
 
     public static class LocalLoggerContextFactory extends TestLoggerContextFactory {}
@@ -73,4 +165,14 @@ public LocalProvider() {
             super(0, CURRENT_VERSION, LocalLoggerContextFactory.class);
         }
     }
+
+    private void assertHasErrorOrWarning(final TestLogger statusLogger) {
+        assertThat(statusLogger.getEntries()).as("check StatusLogger entries").anySatisfy(entry -> assertThat(entry)
+                .matches(ERROR_OR_WARNING));
+    }
+
+    private void assertNoErrorsOrWarnings(final TestLogger statusLogger) {
+        assertThat(statusLogger.getEntries()).as("check StatusLogger entries").allSatisfy(entry -> assertThat(entry)
+                .doesNotMatch(ERROR_OR_WARNING));
+    }
 }

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

@@ -48,7 +48,7 @@ public DefaultThreadContextMap() {
     }
 
     /**
-     * @deprecated Since 2.24.0 use the default constructor or {@link NoOpThreadContextMap} instead.
+     * @deprecated Since 2.24.0. See {@link Provider#getThreadContextMap()} on how to obtain a no-op map.
      */
     @Deprecated
     public DefaultThreadContextMap(final boolean useMap) {

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

@@ -24,7 +24,10 @@
  * {@code disableThreadContext} is {@code true}. This implementation does nothing.
  *
  * @since 2.7
+ * @deprecated since 2.24.0. Return the {@value Provider#NO_OP_CONTEXT_MAP} constant in
+ * {@link Provider#getThreadContextMap()} instead.
  */
+@Deprecated
 public class NoOpThreadContextMap implements ThreadContextMap {
     @Override
     public void clear() {}