AbstractConfiguration - Potential NPE on AsyncLoggerConfigDisruptor

AbstractConfiguration (Log4j 2.24.1)

In AbstractConfiguration the field asyncLoggerConfigDisruptoris lazily initialized.

private AsyncLoggerConfigDisruptor asyncLoggerConfigDisruptor;

public AsyncLoggerConfigDelegate getAsyncLoggerConfigDelegate() {
    // lazily instantiate only when requested by AsyncLoggers:
    // loading AsyncLoggerConfigDisruptor requires LMAX Disruptor jar on classpath
    if (asyncLoggerConfigDisruptor == null) {
        asyncLoggerConfigDisruptor = new AsyncLoggerConfigDisruptor(asyncWaitStrategyFactory);
    }
    return asyncLoggerConfigDisruptor;
}

The method getAsyncLoggerConfigDelegate() would normally be called in the constructor of AsyncLoggerConfig(...):

protected AsyncLoggerConfig(
            final String name,
            final List<AppenderRef> appenders,
            final Filter filter,
            final Level level,
            final boolean additive,
            final Property[] properties,
            final Configuration config,
            final boolean includeLocation) {
        super(name, appenders, filter, level, additive, properties, config, includeLocation);
        delegate = config.getAsyncLoggerConfigDelegate();
        delegate.setLogEventFactory(getLogEventFactory());
    }

Here an assumption is being made that Configuration#getAsyncLoggerConfigDelegate() will not return null and properly initialiize the field, but since Configuration is an interface that can be implemented by anyone, anything is possible. :) It also assumes that the AsyncLoggerConfig has been created with this AbstractConfiguration which may not be a given if created somewhere else and just added with Configuration#addLogger(String, LoggerConfig).

This also implies that the AbstractConfiguration has a certain "knowledge" of the inner workings of the AsyncLoggerConfig which I believe violates some principle of encapsulation. :)

The potential NPE comes up in the AbstractConfiguration#start() and AbstractConfiguration#stop() methods where it is assumed that the asyncLoggerConfigDisruptor has already been lazily initialized to a non-null value.

start()

if (hasAsyncLoggers()) {
    asyncLoggerConfigDisruptor.start();
}

stop()

if (hasAsyncLoggers()) {
    LOGGER.trace("{} stopping AsyncLoggerConfigDisruptor.", cls);
    asyncLoggerConfigDisruptor.stop(timeout, timeUnit);
}

A potential fix is either doing a simple null-check or maybe adding a protected API (since you can't call start() on the AsyncLoggerConfigDelegate:

@Override
public AsyncLoggerConfigDelegate getAsyncLoggerConfigDelegate() {
   return getAsyncLoggerConfigDisruptor();
}

protected AsyncLoggerConfigDisruptor getAsyncLoggerConfigDisruptor() {
    // lazily instantiate only when requested by AsyncLoggers:
    // loading AsyncLoggerConfigDisruptor requires LMAX Disruptor jar on classpath
    if (asyncLoggerConfigDisruptor == null) {
        asyncLoggerConfigDisruptor = new AsyncLoggerConfigDisruptor(asyncWaitStrategyFactory);
    }
    return asyncLoggerConfigDisruptor;
}

public void start() {
    ...
    if (hasAsyncLoggers()) {
        getAsyncLoggerConfigDisruptor().start();
    }
    ...
}

public void stop() {
    ...
    if (hasAsyncLoggers()) {
        getAsyncLoggerConfigDisruptor().stop(timeout, timeUnit);
    }
    ...
}

apache / logging-log4j2

AbstractConfiguration - Potential NPE on AsyncLoggerConfigDisruptor #3156