On Wed, 23 Aug 2023 15:41: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.
src/java.base/share/classes/jdk/internal/logger/BootstrapLogger.java line 230:
> 228: // The accessor in which this logger is temporarily set.
> 229: final LazyLoggerAccessor holder;
> 230: final BooleanSupplier isLoadingThread;
Suggestion:
// tests whether the logger is invoked by the loading thread before
// the LoggerFinder is loaded; can be null;
final BooleanSupplier isLoadingThread;
src/java.base/share/classes/jdk/internal/logger/BootstrapLogger.java line 234:
> 232: boolean isLoadingThread() {
> 233: return isLoadingThread != null && isLoadingThread.getAsBoolean();
> 234: }
Suggestion:
// returns true if the logger is invoked by the loading thread before the
// LoggerFinder service is loaded
boolean isLoadingThread() {
return isLoadingThread != null && isLoadingThread.getAsBoolean();
}
src/java.base/share/classes/jdk/internal/logger/BootstrapLogger.java line 948:
> 946: // and the LogManager has not been initialized yet.
> 947: public static boolean useLazyLoggers() {
> 948: if (!BootstrapLogger.isBooted() ||
Suggestion:
// Note: avoid triggering the initialization of the DetectBackend class
// while holding the BotstrapLogger class monitor
if (!BootstrapLogger.isBooted() ||
src/java.base/share/classes/jdk/internal/logger/LazyLoggers.java line 425:
> 423: */
> 424: public static final Logger getLogger(String name, Module module) {
> 425: BootstrapLogger.detectBackend();
Suggestion:
// triggers detection of the backend
BootstrapLogger.detectBackend();
src/java.base/share/classes/jdk/internal/logger/LoggerFinderLoader.java line 76:
> 74:
> 75: // Return the loaded LoggerFinder, or load it if not already loaded.
> 76: static volatile Thread loadingThread;
Suggestion:
// record the loadingThread while loading the backend
static volatile Thread loadingThread;
// Return the loaded LoggerFinder, or load it if not already loaded.
src/java.base/share/classes/jdk/internal/logger/LoggerFinderLoader.java line 83:
> 81: Thread currentThread = Thread.currentThread();
> 82: if (loadingThread == currentThread) {
> 83: return new TemporaryLoggerFinder();
Suggestion:
// recursive ttempt to load the backend while loading the
backend
// use a temporary logger finder that returns special
BootsrtapLoggers
// which will wait until loading is finished
return new TemporaryLoggerFinder();
src/java.base/share/classes/jdk/internal/logger/LoggerFinderLoader.java line 98:
> 96: }
> 97:
> 98: static boolean isLoadingThread() {
Suggestion:
// returns true if called by the thread that loads the LoggerFinder, while
// loading the LoggerFinder.
static boolean isLoadingThread() {
src/java.base/share/classes/jdk/internal/logger/LoggerFinderLoader.java line
140:
> 138: }
> 139:
> 140: static class TemporaryLoggerFinder extends LoggerFinder {
Suggestion:
private static final class TemporaryLoggerFinder extends LoggerFinder {
src/java.base/share/classes/jdk/internal/logger/LoggerFinderLoader.java line
145:
> 143: @Override
> 144: public Logger apply(String name, Module module) {
> 145: return LazyLoggers.getLoggerFromFinder(name,
> module);
A better idea might be to:
Suggestion:
return LazyLoggers.getLogger(name, module);
src/java.base/share/classes/jdk/internal/logger/LoggerFinderLoader.java line
154:
> 152: }
> 153: };
> 154:
Suggestion:
private static final TemporaryLoggerFinder INSTANCE = new
TemporaryLoggerFinder();
test/jdk/java/lang/System/LoggerFinder/SignedLoggerFinderTest/SignedLoggerFinderTest.java
line 161:
> 159: Logger testLogger = Logger.getLogger("jdk.event.security");
> 160: assertEquals(testLogger.getClass().getName(),
> "java.util.logging.Logger");
> 161: testComplete = true;
I believe these lines should be after `Security.setProperty("test_2",
"test");`, to make sure the logger is loaded by the module that uses it. Also I
was expecting some kind of checks to verify that the expected messages are
indeed logged.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/15404#discussion_r1303306422
PR Review Comment: https://git.openjdk.org/jdk/pull/15404#discussion_r1303310327
PR Review Comment: https://git.openjdk.org/jdk/pull/15404#discussion_r1303314153
PR Review Comment: https://git.openjdk.org/jdk/pull/15404#discussion_r1303315890
PR Review Comment: https://git.openjdk.org/jdk/pull/15404#discussion_r1303317596
PR Review Comment: https://git.openjdk.org/jdk/pull/15404#discussion_r1303320330
PR Review Comment: https://git.openjdk.org/jdk/pull/15404#discussion_r1303323768
PR Review Comment: https://git.openjdk.org/jdk/pull/15404#discussion_r1303327924
PR Review Comment: https://git.openjdk.org/jdk/pull/15404#discussion_r1303324768
PR Review Comment: https://git.openjdk.org/jdk/pull/15404#discussion_r1303326813
PR Review Comment: https://git.openjdk.org/jdk/pull/15404#discussion_r1303342480
