On Wed, 11 Mar 2026 11:08:14 GMT, Daniel Fuchs <[email protected]> wrote:
> The `ParentLoggerTest` is supposed to verify that when a logger is created > programatically, any parent logger for which a level or handler is defined in > the configuration will be created along if not already created. > > In the `LogManager` class, the method `addLocalLogger` calls the > `processParentHandlers` method to process the parent hierarchy and potentialy > create any loggers from that hierarchy that is defined in the configuration > but that has not been created yet, adding them to the logger tree. However, > these loggers newly created by `processParentHandlers` are only weakly > referenced by the tree, until `addLocalLogger` later sets-up the `parent` > field in their children (a strong reference to the parent logger). There is > therefore a short time window where loggers defined in the configuration can > be lazily created by `processParentHandlers` but immediately garbage > collected before the strong reference that should keep them alive when their > child is strongly referenced has been set. > > The `java/util/logging/ParentLoggersTest.java` has been observed failing > intermitently due to that, first in the loom repo, and now again in the > valhalla repo. > > I was able to reproduce the failure 100% of the time by adding a call to > `System.gc()` in the `addLocalLogger` method, calling it in a loop, waiting > until a weak reference was cleared, right after calling > `processParentHandlers` but before starting setting up the child/parent > relationship. > > The fix proposed here makes uses of the `visited` predicate that can be > passed to the `processParentHandlers` method. > `addLocalLogger` now passes a special `visited` predicate that will save the > newly created loggers in a local array list, instead of passing a predicate > that only always returns false as it was before. > That array list is then cleared after the child/parent relationships have > been updated. > This ensures that parent loggers are kept alive until they are strongly > referenced by their child. > > With the fix, and using the modified `addLocalLogger` that calls `System.gc`, > the issue no longer reproduces. > > Testing this reliably requires a modification of the `LogManager` that we > can't keep in the product code, so I believe we will unfortunately have to > rely on the existing `ParentLoggersTest.java` to continue verifying this > issue is fixed. src/java.logging/share/classes/java/util/logging/LogManager.java line 742: > 740: // implementation as a handler class may be only visible to > LogManager > 741: // subclass for the custom log manager case > 742: processParentHandlers(logger, name, VisitedLoggers.NEVER); After this change, does the field `VisitedLoggers.NEVER` end up being unused? Should we remove it? src/java.logging/share/classes/java/util/logging/LogManager.java line 754: > 752: // implementation as a handler class may be only visible > to LogManager > 753: // subclass for the custom log manager case > 754: processParentHandlers(logger, name, visited); This looks reasonable to me. There's one more location where processParentHandlers(...) is getting called in this class. Do you think that would need similar attention? ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/30185#discussion_r2919505848 PR Review Comment: https://git.openjdk.org/jdk/pull/30185#discussion_r2919517507
