Missing/Faulty Logic in AbstractConfiguration for addLoggerAppender / addLoggerFilter / setLoggerAdditive

[JWT007]

AbstractConfiguration (Log4j 2.24.1)

It seems there is some missing/faulty logic in the AbstractConfiguration for the methods addLoggerAppender, addLoggerFilter, setLoggerAdditive.

These methods all use the Map.putIfAbsent(key, value) to only add a new element if not already present; however, they do not check if the value was actually added.

I will just show one example here but they all have the same problem.

public synchronized void addLoggerAppender(
            final org.apache.logging.log4j.core.Logger logger, final Appender appender) {
        if (appender == null || logger == null) {
            return;
        }
        final String loggerName = logger.getName();
        appenders.putIfAbsent(appender.getName(), appender);   // <==== HERE!
        final LoggerConfig lc = getLoggerConfig(loggerName);
        if (lc.getName().equals(loggerName)) {
            lc.addAppender(appender, null, null);
        } else {
            final LoggerConfig nlc = new LoggerConfig(loggerName, lc.getLevel(), lc.isAdditive());
            nlc.addAppender(appender, null, null);
            nlc.setParent(lc);
            loggerConfigs.putIfAbsent(loggerName, nlc);  // <==== AND HERE!
            setParents();
            logger.getContext().updateLoggers();
        }
    }

Here the Appender is added if absent. If it however was not absent, it is not added to the configuration but is added to the LoggerConfig if the name matches. If the name does not match a new LoggerConfig is created and again putIfAbsent is called - and if not added the parents are still set and the context loggers are still updated (which would be a unnecessary operation if not added).

I think this could be optimized, for example, as follows:

NOTE: Map.putIfAbsent(k,v)returns nullif the key did not previously exist and the value was added to the map. If a value already exists for the key, it returns the existing value.

public synchronized void addLoggerAppender(final @Nullable Logger logger, final @Nullable Appender appender) {

   if (appender == null || logger == null) {
     return;
   }

   final String loggerName = logger.getName();

   if (appenders.putIfAbsent(appender.getName(), appender) != null) {
     return;  // <=== IF NOT ADDED, THEN RETURN -- NOTHING MORE CAN BE DONE - EXISTING APPENDER W/SAME NAME
   }

   final LoggerConfig lc = getLoggerConfig(loggerName);

   if (lc.getName().equals(loggerName)) {

     lc.addAppender(appender, null, null);

   } else {

     final LoggerConfig nlc = new LoggerConfig(loggerName, lc.getLevel(), lc.isAdditive());
     nlc.addAppender(appender, null, null);
     nlc.setParent(lc);

     // ONLY UPDATE LOGGER PARENTS AND CONTEXT IF ACTUALLY ADDED
     //    NOTE: One can assume that adding will succeed since syncronized and not found above - but this is a safety check
     if (loggerConfigs.putIfAbsent(loggerName, nlc) == null) {  
       setLoggerParents();
       logger.getContext().updateLoggers();
     }

   }

 }

The same fix could be applied to the other two methods.

Side note 1: addLoggerFilter / setLoggerAdditive do not check if Logger is null - potential NPE.

Side note 2: This class is also a bit inconsistent about sometimes exposing internal collections and sometimes returning immutable collections.

i.e.:

public Map<String, Appender> getAppenders() {
    return appenders;
}
public Map<String, LoggerConfig> getLoggers() {
   return Collections.unmodifiableMap(loggerConfigs);
}

[JWT007]

NOTE: The only "in-library" problem with making getAppenders() return an immutable map is in two unit tests:

• ThreadContextBenchmark2#LogBaselineState
• ThreadContextBenchmark2#LogThreadContextState

Each do the following:

config.getAppenders().forEach((name, app) -> app.stop());
config.getAppenders().clear();

Maybe stopping the existing appenders is suffficient for these tests and the entire map does not need to be cleared?