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
