vy commented on code in PR #3199:
URL: https://github.com/apache/logging-log4j2/pull/3199#discussion_r1840646776
##########
log4j-api/src/main/java/org/apache/logging/log4j/spi/LoggerRegistry.java:
##########
@@ -43,7 +41,7 @@
@NullMarked
public class LoggerRegistry<T extends ExtendedLogger> {
- private final Map<String, Map<MessageFactory, WeakReference<T>>>
loggerRefByMessageFactoryByName = new HashMap<>();
+ private final Map<MessageFactory, Map<String, T>>
loggerByNameByMessageFactory = new WeakHashMap<>();
Review Comment:
This PR, instead of fixing _"the immediate problem"_, rewrites the
`LoggerRegistry` and reverts the behavior to scope the lifetime of `Logger`s
with the associated `MF`'s lifetime. I prefer to preserve the old (yet buggy)
behavior that `Logger`s will be GC'ed when they become unreachable. IMO, _the
fix_ is two lines of code: adding `Reference.reachabilityFence(logger)` before
`return logger` in `putIfAbsent()` and `computeIfAbsent()`. Though
`Reference.reachabilityFence()` is introduced in Java 9. We can try imitating
it with a usage of the value to force a strong reference (e.g.,
`logger.log(Level.OFF, "nada")`), but VM can get this inlined/eliminated, and
we are back to square one. Anyway, I am looking at this. I will see what else
can be done.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
