JWT007 opened a new issue, #3155:
URL: https://github.com/apache/logging-log4j2/issues/3155
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 `null`if 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);
if (loggerConfigs.putIfAbsent(loggerName, nlc) == null) { // ONLY
UPDATE LOGGER PARENTS AND CONTEXT IF ACTUALLY ADDED
setLoggerParents();
logger.getContext().updateLoggers();
}
}
}
```
The same *fix* could be applied to the other two methods.
Side note: 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);
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@logging.apache.org.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
