Improve Loggers.useConsoleLoggers() thread safety

[alexismanin]

Fix for #3170

Rework ConsoleLoggerFactory cache to provide a more consistent behavior.

Notes:

• As a cache strategy, I've chosen synchronized keyword + WeakHashMap combination. I've made this choice as a tradeoff between code complexity and proper cache eviction. There might be better solutions, maybe you have suggestions ?
  • I've thought about using StampedLock instead of synchronized, but I decided to leave it aside and provide a simpler code for first review
• I've added a regression test for the concurrent error, but I would appreciate advise on how I could improve it because:
  • Failure is inconsistent due to the concurrent nature of the problem. I could add a loop to the test, to force failure behavior, but I'm afraid that it would make it too long to execute.
  • Maybe it is too heavy, as it starts its own thread-pool.
• Independently from the current issue, I've seen that there's a duplicated function format used 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.

[alexismanin]

I've reworked my "enhancement" commit, because the way I've used WeakHashMap initially was totally flawed.

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]

good improvement on the WeakHashMap usage. found one improvement around ConsoleLoggerKey.verbose

[simonbasle]

why duplicate the verbose field? local field could be removed in favor of this.identifier.verbose wdyt?

[alexismanin]

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.

[simonbasle]

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 👍 )

[reactorbot]

@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 main 🙇