ppkarwasz commented on code in PR #3199:
URL: https://github.com/apache/logging-log4j2/pull/3199#discussion_r1842914829
##########
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()`.
Currenlty the `Logger` is GC-ed between the call to `putIfAbsent()` and the
call to `getLogger()`. Unless I am mistaken, `reachabilityFence` will not help
with that. Therefore the refactoring from `Map<String, Map<MessageFactory,
WeakReference<T>>>` to `Map<String, Map<MessageFactory, T>>` is required.
The refactoring from `Map<String, Map<MessageFactory, T>>` to
`Map<MessageFactory, Map<String, T>>` is indeed not needed. Do you object to
this change? I changed this to limit the number of `Map` instances. In the
typical situation, when there is only one `MessageFactory` instance, the
previous implementation would create one internal `Map` per logger name. The
proposed one creates one internal `Map` per message factory.
--
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]
