Re: RFR: 8314263: Signed jars triggering Logger finder recursion and StackOverflowError [v7]
On Tue, 29 Aug 2023 15:31:03 GMT, Sean Coffey 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 one additional
> commit since the last revision:
>
> more tidying up in tests
test/jdk/java/lang/System/LoggerFinder/SignedLoggerFinderTest/SignedLoggerFinderTest.java
line 214:
> 212: private static Runnable runnableWithSleep(Supplier s, long sleep,
> String desc) {
> 213: return () -> {
> 214: while(!testComplete) {
Suggestion:
while (!testComplete) {
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15404#discussion_r1309949039
Re: RFR: 8314263: Signed jars triggering Logger finder recursion and StackOverflowError [v7]
> 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 one additional commit since the last revision: more tidying up in tests - Changes: - all: https://git.openjdk.org/jdk/pull/15404/files - new: https://git.openjdk.org/jdk/pull/15404/files/cf53841b..f8d7545e Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=15404&range=06 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=15404&range=05-06 Stats: 19 lines in 4 files changed: 0 ins; 8 del; 11 mod Patch: https://git.openjdk.org/jdk/pull/15404.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15404/head:pull/15404 PR: https://git.openjdk.org/jdk/pull/15404
Re: RFR: 8314263: Signed jars triggering Logger finder recursion and StackOverflowError [v6]
On Tue, 29 Aug 2023 11:58:14 GMT, Sean Coffey 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 one additional > commit since the last revision: > > Review comments from Daniel. Further test clean up Only minor comment updates. Otherwise LGTM! test/jdk/java/lang/System/LoggerFinder/RecursiveLoading/PlatformRecursiveLoadingTest.java line 46: > 44: > 45: /** > 46: * This test triggers recursion by calling `System.getLogger` in the > class init Suggestion: * This test triggers recursion by calling `System.getLogger` in the class init and constructor test/jdk/java/lang/System/LoggerFinder/RecursiveLoading/RecursiveLoadingTest.java line 41: > 39: > 40: /** > 41: * This test triggers recursion by calling `System.getLogger` in the > class init Suggestion: * This test triggers recursion by calling `System.getLogger` in the class init and constructor - Marked as reviewed by dfuchs (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/15404#pullrequestreview-1600360266 PR Review Comment: https://git.openjdk.org/jdk/pull/15404#discussion_r1308784213 PR Review Comment: https://git.openjdk.org/jdk/pull/15404#discussion_r1308785012
Re: RFR: 8314263: Signed jars triggering Logger finder recursion and StackOverflowError [v6]
> 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 one additional commit since the last revision: Review comments from Daniel. Further test clean up - Changes: - all: https://git.openjdk.org/jdk/pull/15404/files - new: https://git.openjdk.org/jdk/pull/15404/files/ca4cd4f0..cf53841b Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=15404&range=05 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=15404&range=04-05 Stats: 43 lines in 4 files changed: 7 ins; 24 del; 12 mod Patch: https://git.openjdk.org/jdk/pull/15404.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15404/head:pull/15404 PR: https://git.openjdk.org/jdk/pull/15404
Re: RFR: 8314263: Signed jars triggering Logger finder recursion and StackOverflowError [v5]
On Mon, 28 Aug 2023 18:49:40 GMT, Daniel Fuchs wrote: >> Sean Coffey has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Fix up test cases > > test/jdk/java/lang/System/LoggerFinder/RecursiveLoading/PlatformRecursiveLoadingTest.java > line 64: > >> 62: >> logs.stream().map(SimpleLogRecord::of).forEach(System.out::println); >> 63: >> logs.stream().map(SimpleLogRecord::of).forEach(SimpleLogRecord::check); >> 64: assertEquals(String.valueOf(logs.size()), String.valueOf(2)); > > Suggestion: > > assertEquals(String.valueOf(logs.size()), String.valueOf(3)); > > See suggestions to SimpleLoggerFinder below... Creating a logger from within the SimpleLoggerFinder constructor ensures that we get the expected StackOverFlow when the patch is not present. I believe it's a better emulation for the issue that was detected with signed jars, where a logger was created while loading the provider. - PR Review Comment: https://git.openjdk.org/jdk/pull/15404#discussion_r1307798105
Re: RFR: 8314263: Signed jars triggering Logger finder recursion and StackOverflowError [v5]
On Mon, 28 Aug 2023 18:45:03 GMT, Sean Coffey 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 one additional > commit since the last revision: > > Fix up test cases test/jdk/java/lang/System/LoggerFinder/RecursiveLoading/PlatformRecursiveLoadingTest.java line 64: > 62: > logs.stream().map(SimpleLogRecord::of).forEach(System.out::println); > 63: > logs.stream().map(SimpleLogRecord::of).forEach(SimpleLogRecord::check); > 64: assertEquals(String.valueOf(logs.size()), String.valueOf(2)); Suggestion: assertEquals(String.valueOf(logs.size()), String.valueOf(3)); See suggestions to SimpleLoggerFinder below... - PR Review Comment: https://git.openjdk.org/jdk/pull/15404#discussion_r1307785939
Re: RFR: 8314263: Signed jars triggering Logger finder recursion and StackOverflowError [v4]
On Mon, 28 Aug 2023 17:29:16 GMT, Sean Coffey 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 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 LOGS = new
> CopyOnWriteArrayList<>();
> 50:
Suggestion:
private final Map 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/j
Re: RFR: 8314263: Signed jars triggering Logger finder recursion and StackOverflowError [v4]
On Mon, 28 Aug 2023 17:42:34 GMT, Daniel Fuchs wrote: >> 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 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? BTW: With the modification suggested below in SimpleLoggerFinder, note that the expected number of messages should be 3 (not 2). - PR Review Comment: https://git.openjdk.org/jdk/pull/15404#discussion_r1307772571
Re: RFR: 8314263: Signed jars triggering Logger finder recursion and StackOverflowError [v5]
> 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 one additional commit since the last revision: Fix up test cases - Changes: - all: https://git.openjdk.org/jdk/pull/15404/files - new: https://git.openjdk.org/jdk/pull/15404/files/83fcfae8..ca4cd4f0 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=15404&range=04 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=15404&range=03-04 Stats: 6 lines in 2 files changed: 0 ins; 3 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/15404.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15404/head:pull/15404 PR: https://git.openjdk.org/jdk/pull/15404
Re: RFR: 8314263: Signed jars triggering Logger finder recursion and StackOverflowError [v4]
> 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 - Changes: - all: https://git.openjdk.org/jdk/pull/15404/files - new: https://git.openjdk.org/jdk/pull/15404/files/3b35c440..83fcfae8 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=15404&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=15404&range=02-03 Stats: 389 lines in 9 files changed: 366 ins; 10 del; 13 mod Patch: https://git.openjdk.org/jdk/pull/15404.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15404/head:pull/15404 PR: https://git.openjdk.org/jdk/pull/15404
Re: RFR: 8314263: Signed jars triggering Logger finder recursion and StackOverflowError [v3]
On Fri, 25 Aug 2023 10:33:49 GMT, Jaikiran Pai wrote:
>> 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();
>
> Hello Daniel, Sean, I couldn't understand the need for this method. The
> changes to `BootstrapLogger` in this PR removes the initialization of
> `DetectBackend` class while holding a lock on `BootstrapLogger` class in the
> `BootstrapLogger.useLazyLoggers` method. Wouldn't that be enough?
a deadlock is still possible Jai with forcing class initialization here. The
new auto test confirms.
here's the interesting calling stack :
"Thread-0" #31 [2241170] prio=5 os_prio=0 cpu=32.77ms elapsed=13.69s
tid=0x7fb0b019bf10 nid=2241170 waiting on condition [0x7fb01ac29000]
java.lang.Thread.State: RUNNABLE
at
jdk.internal.logger.BootstrapLogger.useLazyLoggers(java.base@22-internal/BootstrapLogger.java:952)
- waiting on the Class initialization monitor for
jdk.internal.logger.BootstrapLogger$DetectBackend
at
jdk.internal.logger.LazyLoggers.getLazyLogger(java.base@22-internal/LazyLoggers.java:462)
at
jdk.internal.logger.LazyLoggers.getLogger(java.base@22-internal/LazyLoggers.java:437)
at java.lang.System.getLogger(java.base@22-internal/System.java:1822)
at
jdk.internal.event.EventHelper.isLoggingSecurity(java.base@22-internal/EventHelper.java:148)
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15404#discussion_r1305856639
Re: RFR: 8314263: Signed jars triggering Logger finder recursion and StackOverflowError [v2]
On Fri, 25 Aug 2023 10:24:23 GMT, Jaikiran Pai wrote:
>> Sean Coffey has updated the pull request incrementally with two additional
>> commits since the last revision:
>>
>> - Improve test coverage
>> - Incorporate review comments from Daniel
>
> src/java.base/share/classes/jdk/internal/logger/BootstrapLogger.java line 988:
>
>> 986: private static void ensureClassInitialized(Class c) {
>> 987: try {
>> 988: MethodHandles.lookup().ensureInitialized(c);
>
> Hello Sean, should we check if there are any implications, like on startup
> performance, of using `MethodHandles` in this `BootstrapLogger`?
Thanks for the comments Jai. Latest patch just pushed resolved this - in any
case, it looks like the MethodHandles class is loaded very early in the module
system (even before the application code)
[0.026s][info][class,load] java.util.HexFormat source: shared objects file
[0.026s][info][class,load] java.util.concurrent.atomic.AtomicInteger source:
shared objects file
[0.026s][info][class,load] jdk.internal.module.ModuleBootstrap source: shared
objects file
[0.026s][info][class,load] java.lang.invoke.MethodHandles source: shared
objects file
[0.026s][info][class,load] java.lang.invoke.MemberName$Factory source: shared
objects file
[0.026s][info][class,load] java.lang.invoke.MethodHandles$Lookup source: shared
objects file
[0.027s][info][class,load] java.lang.StrictMath source: shared objects file
[0.032s][info][class,load] java.security.SecureClassLoader$DebugHolder source:
shared objects file
[0.032s][info][class,load] sun.security.util.Debug source: shared objects file
[0.033s][info][class,load] SignedLoggerFinderTest source:
file:/MYREPO/jdk/open/test/jdk/JTwork/classes/0/java/lang/System/LoggerFinder/SignedLoggerFinderTest/SignedLoggerFinderTest.d/
[0.033s][info][class,load] java.lang.NamedPackage source: shared objects file
[0.033s][info][class,load] jdk.internal.misc.MainMethodFinder source: shared
objects file
[0.033s][info][class,load] jdk.internal.misc.PreviewFeatures source: shared
objects file
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15404#discussion_r1305854273
Re: RFR: 8314263: Signed jars triggering Logger finder recursion and StackOverflowError [v3]
> 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 one additional commit since the last revision: fix up bootstrap loggers, patch contribution from Daniel - Changes: - all: https://git.openjdk.org/jdk/pull/15404/files - new: https://git.openjdk.org/jdk/pull/15404/files/976fdb27..3b35c440 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=15404&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=15404&range=01-02 Stats: 70 lines in 3 files changed: 23 ins; 32 del; 15 mod Patch: https://git.openjdk.org/jdk/pull/15404.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15404/head:pull/15404 PR: https://git.openjdk.org/jdk/pull/15404
Re: RFR: 8314263: Signed jars triggering Logger finder recursion and StackOverflowError [v2]
On Thu, 24 Aug 2023 10:54:19 GMT, Sean Coffey 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: > > - Improve test coverage > - Incorporate review comments from Daniel Thanks for your comments @jai - I'll incorporate your test comments soon. - PR Comment: https://git.openjdk.org/jdk/pull/15404#issuecomment-1693595951
Re: RFR: 8314263: Signed jars triggering Logger finder recursion and StackOverflowError [v2]
On Thu, 24 Aug 2023 10:54:19 GMT, Sean Coffey 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:
>
> - Improve test coverage
> - Incorporate review comments from Daniel
test/jdk/java/lang/System/LoggerFinder/SignedLoggerFinderTest/SignedLoggerFinderTest.java
line 122:
> 120: Thread.sleep(sleep);
> 121: } catch (InterruptedException e) {
> 122: throw new RuntimeException(e);
Given that this will end up being thrown from a `Thread`, this will end up
being an uncaught exception and handled by a `UncaughtExceptionHandler` (I
don't remember if/what jtreg sets it to). The default `ThreadGroup`
UncaughtExceptionHandler, just logs to System.err such exceptions
https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/lang/ThreadGroup.java#L697.
Just noting it here, in case you want to do this differently if you want this
exception to fail the test.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15404#discussion_r1305534996
Re: RFR: 8314263: Signed jars triggering Logger finder recursion and StackOverflowError [v2]
On Thu, 24 Aug 2023 10:54:19 GMT, Sean Coffey 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:
>
> - Improve test coverage
> - Incorporate review comments from Daniel
test/jdk/java/lang/System/LoggerFinder/SignedLoggerFinderTest/SignedLoggerFinderTest.java
line 109:
> 107:
> Boolean.parseBoolean(System.getProperty("mutliThreadLoad", "false"));
> 108: boolean withCustomLoggerFinder =
> 109:
> Boolean.parseBoolean(System.getProperty("withCustomLoggerFinder", "false"));
Nit - these two calls can be replaced with
`Boolean.getBoolean("")`. If you however want to use this current
form, that's fine too.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15404#discussion_r1305525527
Re: RFR: 8314263: Signed jars triggering Logger finder recursion and StackOverflowError [v2]
On Wed, 23 Aug 2023 17:16:15 GMT, Daniel Fuchs wrote: > We could create a singleton instance of TemporaryLoggerFinder in the > TemporaryLoggerFinder class and return that. I think the disadvantage is that this singleton instance will never be GCed (till `LoggerFinderLoader` class itself is unloaded) unlike just using a new instance at the call site. Having said that, not being GCed doesn't make a huge difference given what this singleton instance does or holds on to. - PR Review Comment: https://git.openjdk.org/jdk/pull/15404#discussion_r1305512530
Re: RFR: 8314263: Signed jars triggering Logger finder recursion and StackOverflowError [v2]
On Wed, 23 Aug 2023 17:10:42 GMT, Daniel Fuchs wrote:
>> Sean Coffey has updated the pull request incrementally with two additional
>> commits since the last revision:
>>
>> - Improve test coverage
>> - Incorporate review comments from Daniel
>
> 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();
Hello Daniel, Sean, I couldn't understand the need for this method. The changes
to `BootstrapLogger` in this PR removes the initialization of `DetectBackend`
class while holding a lock on `BootstrapLogger` class in the
`BootstrapLogger.useLazyLoggers` method. Wouldn't that be enough?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15404#discussion_r1305493743
Re: RFR: 8314263: Signed jars triggering Logger finder recursion and StackOverflowError [v2]
On Thu, 24 Aug 2023 10:54:19 GMT, Sean Coffey 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:
>
> - Improve test coverage
> - Incorporate review comments from Daniel
src/java.base/share/classes/jdk/internal/logger/BootstrapLogger.java line 988:
> 986: private static void ensureClassInitialized(Class c) {
> 987: try {
> 988: MethodHandles.lookup().ensureInitialized(c);
Hello Sean, should we check if there are any implications, like on startup
performance, of using `MethodHandles` in this `BootstrapLogger`?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15404#discussion_r1305485295
Re: RFR: 8314263: Signed jars triggering Logger finder recursion and StackOverflowError
On Wed, 23 Aug 2023 15:41:16 GMT, Sean Coffey 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.
Thanks for the feedback @dfuch - good to have decent comments in such code.
Good call on using `LazyLoggers.getLogger` in place of
`LazyLoggers.getLoggerFromFinder` also. Changes implemented.
the Security.setProperty("test_2", "test") call is made after loading of the
LoggerFinder framework should be complete. It's there for a sanity check. I
also have Logger class type checks in the test. I've increased coverage in this
area with run test in multi-thread/non multi-thread and customer logger finder
versus default logger finder mode.
-
PR Comment: https://git.openjdk.org/jdk/pull/15404#issuecomment-1691455879
Re: RFR: 8314263: Signed jars triggering Logger finder recursion and StackOverflowError [v2]
> 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: - Improve test coverage - Incorporate review comments from Daniel - Changes: - all: https://git.openjdk.org/jdk/pull/15404/files - new: https://git.openjdk.org/jdk/pull/15404/files/b7cf2947..976fdb27 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=15404&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=15404&range=00-01 Stats: 168 lines in 4 files changed: 110 ins; 38 del; 20 mod Patch: https://git.openjdk.org/jdk/pull/15404.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15404/head:pull/15404 PR: https://git.openjdk.org/jdk/pull/15404
Re: RFR: 8314263: Signed jars triggering Logger finder recursion and StackOverflowError
On Wed, 23 Aug 2023 15:41:16 GMT, Sean Coffey 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
Re: RFR: 8314263: Signed jars triggering Logger finder recursion and StackOverflowError
On Wed, 23 Aug 2023 17:15:03 GMT, Daniel Fuchs 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/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();
We could create a singleton instance of TemporaryLoggerFinder in the
TemporaryLoggerFinder class and return that.
Suggestion:
return new TemporaryLoggerFinder.INSTANCE;
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15404#discussion_r1303321618
