logging-log4j2

logging-log4j2 copied to clipboard

Note: Made two attempts to fix this, but the latter seems a little better. Changes:

• Change the ShutdownCallbackRegistry API back to "register it, we'll call it" - no special GC semantics to worry about, and no need to fix random other uses of the API.
• Move the leak-avoidance Reference semantics out of ShutdownCallbackRegistry and into LoggerContext itself. Since the SoftReference is now to the LoggerContext (rather than an inline lambda), the callback will stay "alive" as long as the LoggerContext itself does.

This seems like a much more sustainable approach to the leak problem - API stays intuitive, and those who need GC guards can implement it themselves as needed, following this pattern and choosing which key objects to reference.

While I agree that the change to registering a runnable without requiring the caller holds a reference to the acknowledgement is easier to understand, I'm a little worried that this change could impact third party components in unexpected ways (cause memory leaks), so we may want to fix the regression without changing that behavior of ShutdownCallbackRegistry.

@jvz or @remkop (based on commits https://github.com/apache/logging-log4j2/commit/cd5edee067cb577d9d2f7607af41bf1f99da0a60 and https://github.com/apache/logging-log4j2/commit/bee90521bb63e6162a9cbed606adde4242675c62) do you recall the expected behavior of addShutdownCallback around reference strength? Do we intend that the Cancellable receipt prevents the task from being garbage collected, or should callers into addShutdownCallback wrap runnables with weak references themselves? Note that callers outside of log4j-core would risk keeping their classloader alive if they implement their own weak referencing runnable wrapper.

From what I remember, the hard part here was making sure any default shutdown callbacks registered in web apps didn't cause classloader leaks. The shutdown hook is a fun place to debug things since not necessarily all the classes in log4j are loaded at that point (or may be unloaded already if not strongly referenced somewhere), so it's mostly just finding the correct boundary point to prevent memory leaks at both the classpath level as well as stronger classloaders like in servlets. For example, see the NullConfiguration class which works around race conditions like that.

Basically, if a new configuration is loaded, the old one shouldn't cause memory leaks from dangling references to what should be garbage collected objects.

I think the difference between the first and second commit on this PR is whether or not the JeroMqManager class (and related context classes) should be allowed to be unloaded if they're only referenced by the shutdown hook. I'm' not sure whether or not that's important.

I think the approach in the first commit is less risky, so I'm going to merge that after I verify tests pass locally to ensure this makes the 2.14.0 release.

@carterkozak Are you doing something with this?