vy commented on code in PR #3418:
URL: https://github.com/apache/logging-log4j2/pull/3418#discussion_r1937582885
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/util/internal/InternalLoggerRegistry.java:
##########
@@ -147,43 +147,51 @@ public Logger computeIfAbsent(
return logger;
}
+ // Intentionally moving the logger creation outside the write lock,
because:
+ //
+ // - Logger instantiation is expensive (causes contention on the
write-lock)
+ //
+ // - User code might have circular code paths, though through
different threads.
+ // Consider `T1[ILR:computeIfAbsent] -> ... -> T1[Logger::new] ->
... -> T2[ILR::computeIfAbsent]`.
+ // Hence, having logger instantiation while holding a write lock
might cause deadlocks:
+ // https://github.com/apache/logging-log4j2/issues/3252
+ // https://github.com/apache/logging-log4j2/issues/3399
+ //
+ // - Creating loggers without a lock, allows multiple threads to
create loggers in parallel, which also improves
+ // performance.
+ //
+ // Since all loggers with the same parameters are equivalent, we can
safely return the logger from the
+ // thread that finishes first.
+ Logger newLogger = loggerSupplier.apply(name, messageFactory);
+
+ // Report name and message factory mismatch if there are any
+ final String loggerName = newLogger.getName();
+ final MessageFactory loggerMessageFactory =
newLogger.getMessageFactory();
+ if (!loggerName.equals(name) ||
!loggerMessageFactory.equals(messageFactory)) {
+ StatusLogger.getLogger()
+ .error(
+ "Newly registered logger with name `{}` and
message factory `{}`, is requested to be associated with a different name `{}`
or message factory `{}`.\n"
+ + "Effectively the message factory of the
logger will be used and the other one will be ignored.\n"
+ + "This generally hints a problem at the
logger context implementation.\n"
+ + "Please report this using the Log4j
project issue tracker.",
+ loggerName,
+ loggerMessageFactory,
+ name,
+ messageFactory);
+ }
+
// Write lock slow path: Insert the logger
writeLock.lock();
try {
-
- // See if the logger is created by another thread in the meantime
- final Map<String, WeakReference<Logger>> loggerRefByName =
-
loggerRefByNameByMessageFactory.computeIfAbsent(messageFactory, ignored -> new
HashMap<>());
- WeakReference<Logger> loggerRef = loggerRefByName.get(name);
- if (loggerRef != null && (logger = loggerRef.get()) != null) {
- return logger;
+ Map<String, WeakReference<Logger>> loggerRefByName =
loggerRefByNameByMessageFactory.get(messageFactory);
+ // noinspection Java8MapApi (avoid the allocation of lambda passed
to `Map::computeIfAbsent`)
Review Comment:
Very fair point. Implemented in 71b3b1c053da87d0877c3c85f263bbc9f3e1d599.
--
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
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
