Improve Loggers.useConsoleLoggers() thread safety#3174
Improve Loggers.useConsoleLoggers() thread safety#3174simonbasle merged 5 commits intoreactor:3.4.xfrom
Conversation
Currently, Loggers.ConsoleLoggerFactory.apply method uses a plain HashMap as a cache, without using any external synchonisation mechanism. It can cause ConcurrentModificationException error when a user tries to acquire a logger See reactor#3170.
Add a test to ensure that console logger factory cache does not return loggers with outdated "verbose" attribute See reactor#3170.
9caec6e to
831aa6d
Compare
Fix both unit tests previously introduced by reworking cache management: - Synchronize cache access to avoid concurrent modification error (TODO: performance may be improved by using a stamped lock, at the cost of code complexity) - Use "verbose" attribute as a part of the cache key, to avoid cache corruptions - Use new key as a basis for cache entry eviction: replace HashMap with WeakHashMap, to give a chance to remove unused loggers Fixes reactor#3170.
Warn user about expected thread-safety guarantee when providing a custom logger factory function See reactor#3170.
831aa6d to
a7f6a83
Compare
|
I've reworked my "enhancement" commit, because the way I've used I've ensured a saner behavior by using the following snippet repeatedly: package reactor.util;
import reactor.util.Logger;
import reactor.util.Loggers;
public class ConsoleLoggerLeak {
public static void main(String[] args) {
Loggers.useConsoleLoggers();
long i = 0;
while (true) {
Logger logger = Loggers.getLogger(Long.toString(i));
if (i++ % 1_000_000 == 0) {
System.out.print("\r"+logger.getName());
}
Logger bis = Loggers.getLogger(Long.toString(i-1));
if (bis != logger) throw new IllegalStateException("Not cached !");
}
}
} |
simonbasle
left a comment
There was a problem hiding this comment.
good improvement on the WeakHashMap usage. found one improvement around ConsoleLoggerKey.verbose
| this.log = log; | ||
| this.err = err; | ||
| this.verbose = verbose; | ||
| this.verbose = identifier.verbose; |
There was a problem hiding this comment.
why duplicate the verbose field? local field could be removed in favor of this.identifier.verbose wdyt?
There was a problem hiding this comment.
It was just a commodity to avoid changing all if (verbose) with if (identifier.verbose).
But both solutions work for me. If you confirm you want it replaced, I will amend my commit to do the switch.
There was a problem hiding this comment.
yeah I think it makes sense to have only one field. please just add a new commit to the branch. I'll squash everything when merging anyway (but I like the effort to have meaningful small commits in the PR branch 👍 )
The information is already available through the logger key.
|
@simonbasle this PR seems to have been merged on a maintenance branch, please ensure the change is merge-forwarded to intermediate maintenance branches and up to |
Fix for #3170
Rework ConsoleLoggerFactory cache to provide a more consistent behavior.
Notes:
synchronizedkeyword +WeakHashMapcombination. I've made this choice as a tradeoff between code complexity and proper cache eviction. There might be better solutions, maybe you have suggestions ?StampedLockinstead ofsynchronized, but I decided to leave it aside and provide a simpler code for first reviewformatused both on console logger and JDK logger. I wonder if it is really needed on JdkLogger, because this work should be done internally by the logger. If you agree, I can rework that as a bonus in this PR.