Fix-ups due to #2230 review

Author: ppkarwasz

log4j-core/src/main/java/org/apache/logging/log4j/core/async/AsyncLoggerConfig.java

@@ -108,11 +108,9 @@ protected AsyncLoggerConfig(
     @Override
     public void initialize() {
         final Configuration configuration = getConfiguration();
-        DisruptorConfiguration disruptorConfig = configuration.getExtension(DisruptorConfiguration.class);
-        if (disruptorConfig == null) {
-            disruptorConfig = DisruptorConfiguration.newBuilder().build();
-            configuration.addExtension(disruptorConfig);
-        }
+        final DisruptorConfiguration disruptorConfig = configuration.addExtensionIfAbsent(
+                DisruptorConfiguration.class,
+                () -> DisruptorConfiguration.newBuilder().build());
         delegate = disruptorConfig.getAsyncLoggerConfigDelegate();
         delegate.setLogEventFactory(getLogEventFactory());
         super.initialize();

log4j-core/src/main/java/org/apache/logging/log4j/core/async/DisruptorConfiguration.java

@@ -27,6 +27,7 @@
 import org.apache.logging.log4j.plugins.PluginBuilderAttribute;
 import org.apache.logging.log4j.plugins.PluginFactory;
 import org.apache.logging.log4j.status.StatusLogger;
+import org.apache.logging.log4j.util.Lazy;
 import org.apache.logging.log4j.util.LoaderUtil;
 
 @Configurable(printObject = true)
@@ -37,7 +38,8 @@ public final class DisruptorConfiguration extends AbstractLifeCycle implements C
     private static final Logger LOGGER = StatusLogger.getLogger();
 
     private final AsyncWaitStrategyFactory waitStrategyFactory;
-    private AsyncLoggerConfigDisruptor loggerConfigDisruptor;
+    private final Lazy<AsyncLoggerConfigDisruptor> loggerConfigDisruptor =
+            Lazy.lazy(() -> new AsyncLoggerConfigDisruptor(getWaitStrategyFactory()));
 
     private DisruptorConfiguration(final AsyncWaitStrategyFactory waitStrategyFactory) {
         this.waitStrategyFactory = waitStrategyFactory;
@@ -48,26 +50,23 @@ public AsyncWaitStrategyFactory getWaitStrategyFactory() {
     }
 
     public AsyncLoggerConfigDelegate getAsyncLoggerConfigDelegate() {
-        if (loggerConfigDisruptor == null) {
-            loggerConfigDisruptor = new AsyncLoggerConfigDisruptor(waitStrategyFactory);
-        }
-        return loggerConfigDisruptor;
+        return loggerConfigDisruptor.get();
     }
 
     @Override
     public void start() {
-        if (loggerConfigDisruptor != null) {
+        if (loggerConfigDisruptor.isInitialized()) {
             LOGGER.info("Starting AsyncLoggerConfigDisruptor.");
-            loggerConfigDisruptor.start();
+            loggerConfigDisruptor.get().start();
         }
         super.start();
     }
 
     @Override
-    public boolean stop(long timeout, TimeUnit timeUnit) {
-        if (loggerConfigDisruptor != null) {
+    public boolean stop(final long timeout, final TimeUnit timeUnit) {
+        if (loggerConfigDisruptor.isInitialized()) {
             LOGGER.info("Stopping AsyncLoggerConfigDisruptor.");
-            loggerConfigDisruptor.stop(timeout, timeUnit);
+            loggerConfigDisruptor.get().stop(timeout, timeUnit);
         }
         return super.stop(timeout, timeUnit);
     }
@@ -97,36 +96,31 @@ public Builder setWaitFactory(final String waitFactory) {
 
         @Override
         public DisruptorConfiguration build() {
-            final String effectiveClassName = Objects.toString(waitFactory, factoryClassName);
-            final AsyncWaitStrategyFactory effectiveFactory;
-            if (effectiveClassName != null) {
-                effectiveFactory = createWaitStrategyFactory(effectiveClassName);
-            } else {
-                effectiveFactory = null;
-            }
-            if (effectiveFactory != null) {
-                LOGGER.info("Using configured AsyncWaitStrategy factory {}.", effectiveClassName);
-            } else {
-                LOGGER.info("Using default AsyncWaitStrategy factory.");
-            }
-            return new DisruptorConfiguration(effectiveFactory);
+            return new DisruptorConfiguration(
+                    createWaitStrategyFactory(Objects.toString(waitFactory, factoryClassName)));
         }
 
         private static AsyncWaitStrategyFactory createWaitStrategyFactory(final String factoryClassName) {
-            try {
-                return LoaderUtil.newCheckedInstanceOf(factoryClassName, AsyncWaitStrategyFactory.class);
-            } catch (final ClassCastException e) {
-                LOGGER.error(
-                        "Ignoring factory '{}': it is not assignable to AsyncWaitStrategyFactory", factoryClassName);
-                return null;
-            } catch (ReflectiveOperationException | LinkageError e) {
-                LOGGER.warn(
-                        "Invalid implementation class name value: error creating AsyncWaitStrategyFactory {}: {}",
-                        factoryClassName,
-                        e.getMessage(),
-                        e);
-                return null;
+            if (factoryClassName != null) {
+                try {
+                    final AsyncWaitStrategyFactory asyncWaitStrategyFactory =
+                            LoaderUtil.newCheckedInstanceOf(factoryClassName, AsyncWaitStrategyFactory.class);
+                    LOGGER.info("Using configured AsyncWaitStrategy factory {}.", factoryClassName);
+                    return asyncWaitStrategyFactory;
+                } catch (final ClassCastException e) {
+                    LOGGER.error(
+                            "Ignoring factory '{}': it is not assignable to AsyncWaitStrategyFactory",
+                            factoryClassName);
+                } catch (final ReflectiveOperationException | LinkageError e) {
+                    LOGGER.warn(
+                            "Invalid implementation class name value: error creating AsyncWaitStrategyFactory {}: {}",
+                            factoryClassName,
+                            e.getMessage(),
+                            e);
+                }
             }
+            LOGGER.info("Using default AsyncWaitStrategy factory.");
+            return null;
         }
     }
 }

log4j-core/src/main/java/org/apache/logging/log4j/core/config/AbstractConfiguration.java

@@ -21,7 +21,6 @@
 import aQute.bnd.annotation.spi.ServiceConsumer;
 import java.lang.ref.WeakReference;
 import java.util.ArrayList;
-import java.util.Arrays;
 import java.util.Collections;
 import java.util.HashSet;
 import java.util.List;
@@ -91,7 +90,8 @@
 @ServiceConsumer(value = ScriptManagerFactory.class, cardinality = Cardinality.SINGLE, resolution = Resolution.OPTIONAL)
 public abstract class AbstractConfiguration extends AbstractFilterable implements Configuration {
 
-    private static final ConfigurationExtension[] EMPTY_EXTENSIONS = new ConfigurationExtension[0];
+    private static final List<String> EXPECTED_ELEMENTS =
+            List.of("\"Appenders\"", "\"Loggers\"", "\"Properties\"", "\"Scripts\"", "\"CustomLevels\"");
 
     /**
      * The instance factory for this configuration. This may be a child factory to a LoggerContext
@@ -159,7 +159,7 @@ public abstract class AbstractConfiguration extends AbstractFilterable implement
     private final WeakReference<LoggerContext> loggerContext;
     private final PropertyEnvironment contextProperties;
     private final Lock configLock = new ReentrantLock();
-    private ConfigurationExtension[] extensions = EMPTY_EXTENSIONS;
+    private final List<ConfigurationExtension> extensions = new CopyOnWriteArrayList<>();
 
     /**
      * Constructor.
@@ -659,7 +659,7 @@ protected void doConfigure() {
         if (!hasProperties) {
             final Map<String, String> map = this.getComponent(CONTEXT_PROPERTIES);
             final StrLookup lookup = map == null ? null : new PropertiesLookup(map);
-            Interpolator interpolator = interpolatorFactory.newInterpolator(lookup);
+            final Interpolator interpolator = interpolatorFactory.newInterpolator(lookup);
             instanceFactory.injectMembers(interpolator);
             runtimeStrSubstitutor.setVariableResolver(interpolator);
             configurationStrSubstitutor.setVariableResolver(interpolator);
@@ -676,6 +676,10 @@ protected void doConfigure() {
             }
             createConfiguration(child, null);
             if (child.getObject() == null) {
+                LOGGER.warn(
+                        "Configuration element \"{}\" is ignored: try nesting it inside one of: {}.",
+                        child.getName(),
+                        EXPECTED_ELEMENTS);
                 continue;
             }
             if ("Scripts".equalsIgnoreCase(child.getName())) {
@@ -686,40 +690,28 @@ protected void doConfigure() {
                 appenders = child.getObject();
             } else if (child.isInstanceOf(Filter.class)) {
                 addFilter(child.getObject(Filter.class));
-            } else if ("Loggers".equalsIgnoreCase(child.getName())) {
-                final Loggers l = child.getObject();
+            } else if (child.isInstanceOf(Loggers.class)) {
+                final Loggers l = child.getObject(Loggers.class);
                 loggerConfigs = l.getMap();
                 setLoggers = true;
                 if (l.getRoot() != null) {
                     root = l.getRoot();
                     setRoot = true;
                 }
-            } else if ("CustomLevels".equalsIgnoreCase(child.getName())) {
-                final CustomLevels levels = child.getObject(CustomLevels.class);
-                if (levels == null) {
-                    LOGGER.error("Unable to load CustomLevels plugin");
-                } else {
-                    customLevels = levels.getCustomLevels();
-                }
+            } else if (child.isInstanceOf(CustomLevels.class)) {
+                customLevels = child.getObject(CustomLevels.class).getCustomLevels();
             } else if (child.isInstanceOf(CustomLevelConfig.class)) {
                 final List<CustomLevelConfig> copy = new ArrayList<>(customLevels);
                 copy.add(child.getObject(CustomLevelConfig.class));
                 customLevels = copy;
             } else if (child.isInstanceOf(ConfigurationExtension.class)) {
-                final ConfigurationExtension extension = child.getObject(ConfigurationExtension.class);
-                if (extension == null) {
-                    LOGGER.error("Unable to load configuration extension {}.", child.getName());
-                } else {
-                    addExtension(child.getObject());
-                }
+                addExtension(child.getObject(ConfigurationExtension.class));
             } else {
-                final List<String> expected = Arrays.asList(
-                        "\"Appenders\"", "\"Loggers\"", "\"Properties\"", "\"Scripts\"", "\"CustomLevels\"");
                 LOGGER.error(
                         "Unknown object \"{}\" of type {} is ignored: try nesting it inside one of: {}.",
                         child.getName(),
                         child.getObject().getClass().getName(),
-                        expected);
+                        EXPECTED_ELEMENTS);
             }
         }
 
@@ -1154,19 +1146,28 @@ public void setNanoClock(final NanoClock nanoClock) {
     }
 
     @Override
-    public void addExtension(ConfigurationExtension extension) {
-        Objects.requireNonNull(extension);
-        extensions = Arrays.copyOf(extensions, extensions.length + 1);
-        extensions[extensions.length - 1] = extension;
+    public <T extends ConfigurationExtension> T addExtensionIfAbsent(
+            final Class<T> extensionType, final Supplier<? extends T> supplier) {
+        for (final ConfigurationExtension extension : extensions) {
+            if (extensionType.isInstance(extension)) {
+                return extensionType.cast(extension);
+            }
+        }
+        return addExtension(supplier.get());
+    }
+
+    private <T extends ConfigurationExtension> T addExtension(final T extension) {
+        extensions.add(Objects.requireNonNull(extension));
+        return extension;
     }
 
     @Override
-    public <T extends ConfigurationExtension> T getExtension(Class<T> extensionType) {
+    public <T extends ConfigurationExtension> T getExtension(final Class<T> extensionType) {
         T result = null;
         for (final ConfigurationExtension extension : extensions) {
             if (extensionType.isInstance(extension)) {
                 if (result == null) {
-                    result = (T) extension;
+                    result = extensionType.cast(extension);
                 } else {
                     LOGGER.warn(
                             "Multiple configuration elements found for type {}. Only the first will be used.",

log4j-core/src/main/java/org/apache/logging/log4j/core/config/Configuration.java

@@ -238,17 +238,25 @@ default RecyclerFactory getRecyclerFactory() {
     }
 
     /**
-     * Registers a new configuration extension.
-     *
-     * @param extension a configuration extension.
+     * Registers a new configuration extension, if it doesn't exist.
+     * <p>
+     *     To preventing polluting the main configuration element,
+     *     each JAR that wishes to extend the {@link Configuration} should use a single child element.
+     * </p>
+     * @param extensionType the concrete type of the extension,
+     * @param supplier a factory to create a new extension element,
+     * @return the current extension if present or a newly generated one.
      * @since 3.0
+     * @see #getExtension(Class)
      */
-    void addExtension(ConfigurationExtension extension);
+    <T extends ConfigurationExtension> T addExtensionIfAbsent(Class<T> extensionType, Supplier<? extends T> supplier);
 
     /**
      * Returns an extension of the given type.
-     *
-     * @param extensionType a type of extension,
+     * <p>
+     *     Only the first extension of the given type is returned.
+     * </p>
+     * @param extensionType a concrete type of the extension,
      * @return an extension the matches the given type.
      * @since 3.0
      */

log4j-core/src/main/java/org/apache/logging/log4j/core/config/ConfigurationExtension.java

@@ -17,6 +17,6 @@
 package org.apache.logging.log4j.core.config;
 
 /**
- * A {@link ConfigurationExtension} is used to add new child elements to a {@link Configuration}.
+ * Marker interface used by new child elements of a {@link Configuration}.
  */
 public interface ConfigurationExtension {}