On Mon, 28 Aug 2023 17:29:16 GMT, Sean Coffey <coff...@openjdk.org> wrote:
>> Recursive initialization calls possible during loading of LoggerFinder >> service. >> >> This fix detects the recursive call and returns a temporary LoggerFinder >> that is backed by a lazy logger. Automated test case developed to simulate >> loading of an external LoggerFinder service while also having other threads >> poke System.getLogger during this framework initialization. > > Sean Coffey has updated the pull request incrementally with two additional > commits since the last revision: > > - Tidy up SignedLoggerFinderTest.java > - Code contribution from Daniel included. New tests. Fix up extra service > call issues test/jdk/java/lang/System/LoggerFinder/RecursiveLoading/PlatformRecursiveLoadingTest.java line 58: > 56: * of this run). Running test in signed and unsigned jar mode for > sanity coverage. > 57: * The current bug only manifests when jars are signed. > 58: */ Suggestion: /** * This test triggers recursion by calling `System.getLogger` in the class init * of a custom LoggerFinder. Without the fix, this is expected to throw * java.lang.NoClassDefFoundError: Could not initialize class jdk.internal.logger.LoggerFinderLoader$ErrorPolicy * caused by: java.lang.StackOverflowError * */ test/jdk/java/lang/System/LoggerFinder/RecursiveLoading/PlatformRecursiveLoadingTest.java line 65: > 63: > logs.stream().map(SimpleLogRecord::of).forEach(System.out::println); > 64: > logs.stream().map(SimpleLogRecord::of).forEach(SimpleLogRecord::check); > 65: assertEquals(String.valueOf(logs.size()), String.valueOf(2));*/ Why is this code commented? Is it an oversight? test/jdk/java/lang/System/LoggerFinder/RecursiveLoading/RecursiveLoadingTest.java line 55: > 53: * of this run). Running test in signed and unsigned jar mode for > sanity coverage. > 54: * The current bug only manifests when jars are signed. > 55: */ Suggestion: /** * This test triggers recursion by calling `System.getLogger` in the class init * of a custom LoggerFinder. Without the fix, this is expected to throw * java.lang.NoClassDefFoundError: Could not initialize class jdk.internal.logger.LoggerFinderLoader$ErrorPolicy * caused by: java.lang.StackOverflowError * */ test/jdk/java/lang/System/LoggerFinder/RecursiveLoading/RecursiveLoadingTest.java line 61: > 59: > logs.stream().map(SimpleLogRecord::of).forEach(System.out::println); > 60: > logs.stream().map(SimpleLogRecord::of).forEach(SimpleLogRecord::check); > 61: assertEquals(String.valueOf(logs.size()), String.valueOf(2)); See suggested changes to SimpleLoggerFinder Suggestion: assertEquals(String.valueOf(logs.size()), String.valueOf(3)); test/jdk/java/lang/System/LoggerFinder/RecursiveLoading/SimpleLoggerFinder.java line 28: > 26: import java.lang.*; > 27: import java.util.*; > 28: import java.util.concurrent.CopyOnWriteArrayList; Suggestion: import java.util.concurrent.CopyOnWriteArrayList; import java.util.concurrent.ConcurrentHashMap; test/jdk/java/lang/System/LoggerFinder/RecursiveLoading/SimpleLoggerFinder.java line 49: > 47: } > 48: > 49: public static final CopyOnWriteArrayList<Object> LOGS = new > CopyOnWriteArrayList<>(); Suggest to move the declaration before the static block above. test/jdk/java/lang/System/LoggerFinder/RecursiveLoading/SimpleLoggerFinder.java line 50: > 48: > 49: public static final CopyOnWriteArrayList<Object> LOGS = new > CopyOnWriteArrayList<>(); > 50: Suggestion: private final Map<String, SimpleLogger> loggers = new ConcurrentHashMap<>(); public SimpleLoggerFinder() { System.getLogger("dummy") .log(System.Logger.Level.INFO, "Logger finder service created"); } test/jdk/java/lang/System/LoggerFinder/RecursiveLoading/SimpleLoggerFinder.java line 53: > 51: @Override > 52: public System.Logger getLogger(String name, Module module) { > 53: return new SimpleLogger(name); Suggestion: return loggers.computeIfAbsent(name, SimpleLogger::new); We're now getting the "dummy" logger twice, and since its constructor has side effects on its underlying jul logger implementation, we don't want to create two loggers for the same name - as that would cause two identical SimpleHandler instances to be added to the same underlying logger, making the log message appear twice in the LOGS list. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/15404#discussion_r1307771237 PR Review Comment: https://git.openjdk.org/jdk/pull/15404#discussion_r1307725024 PR Review Comment: https://git.openjdk.org/jdk/pull/15404#discussion_r1307773997 PR Review Comment: https://git.openjdk.org/jdk/pull/15404#discussion_r1307773194 PR Review Comment: https://git.openjdk.org/jdk/pull/15404#discussion_r1307776466 PR Review Comment: https://git.openjdk.org/jdk/pull/15404#discussion_r1307774748 PR Review Comment: https://git.openjdk.org/jdk/pull/15404#discussion_r1307775886 PR Review Comment: https://git.openjdk.org/jdk/pull/15404#discussion_r1307777399