[
https://issues.apache.org/jira/browse/LOG4J2-2954?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Henry Tung updated LOG4J2-2954:
-------------------------------
Description:
LoggerContext.setUpShutdownHook() passes a callback to the
DefaultShutdownCallbackRegistry. To avoid a memory leak, the registry only
holds a soft reference to the passed callback.
However, the callback is itself just an inline lambda not-otherwise-referenced.
Even while the context is alive, the callback will soon become eligible for GC
once the SoftReference timer-from-last-access expires. By the time the shutdown
hooks actually run, the references are cleared and the context is not
gracefully closed.
----
Probably surfaced after
[https://github.com/apache/logging-log4j2/commit/bee90521bb63e6162a9cbed606adde4242675c62].
Fixing that leak (which actually kept the callback alive) now makes the
callback completely unreferenced (strongly).
Suggested fix:
* make RegisteredCancellable.hook a strong reference
* make DefaultShutdownCallbackRegistry.hooks a Collection<WeakReference<...>>
This would keep the callback alive as long as the returned registration object
(the Cancellable) is alive, and the LoggerContext does reference that.
----
Note: Unclear if the above is actually a complete solution. "LoggerContext
eligible for GC" doesn't seem to imply "LoggerContext contents all flushed to
disk" - the strong reference from the shutdown hook to the context actually
seems necessary, to guarantee that any buffers in the context will be flushed
even if the context itself happens to become unreferenced.
was:
LoggerContext.setUpShutdownHook() passes a callback to the
DefaultShutdownCallbackRegistry. To "avoid a memory leak", the registry only
holds a soft reference to the passed callback.
However, the callback is itself just an inline lambda not-otherwise-referenced.
Even while the context is alive, the callback will soon become eligible for GC
once the SoftReference timer-from-last-access expires. By the time the shutdown
hooks actually run, the references are cleared and the context is not
gracefully closed.
Probably surfaced after
[https://github.com/apache/logging-log4j2/commit/bee90521bb63e6162a9cbed606adde4242675c62.]
"Fixing" that leak (which actually kept the callback alive) now makes the
callback completely unreferenced (strongly).
An improvement would be to make RegisteredCancellable.hook a strong reference,
and instead to make DefaultShutdownCallbackRegistry.hooks a
Collection<WeakReference<...>>. This would keep the callback alive as long as
the returned registration object (the Cancellable) is alive, and the
LoggerContext does reference that.
------
Note: Unclear if the above is actually a complete solution. "LoggerContext
eligible for GC" doesn't seem to imply "LoggerContext contents all flushed to
disk" - the strong reference from the shutdown hook to the context actually
seems correct in that context, to guarantee that contents will be flushed on
finish.
Instead, re: "memory leak", it seems like there should be a burden on Log4j API
consumers to close old contexts they don't need anymore.
> Shutdown callbacks registered in LoggerContext.setUpShutdownHook() aren't
> strongly referenced
> ---------------------------------------------------------------------------------------------
>
> Key: LOG4J2-2954
> URL: https://issues.apache.org/jira/browse/LOG4J2-2954
> Project: Log4j 2
> Issue Type: Bug
> Reporter: Henry Tung
> Priority: Major
>
> LoggerContext.setUpShutdownHook() passes a callback to the
> DefaultShutdownCallbackRegistry. To avoid a memory leak, the registry only
> holds a soft reference to the passed callback.
> However, the callback is itself just an inline lambda
> not-otherwise-referenced. Even while the context is alive, the callback will
> soon become eligible for GC once the SoftReference timer-from-last-access
> expires. By the time the shutdown hooks actually run, the references are
> cleared and the context is not gracefully closed.
> ----
> Probably surfaced after
> [https://github.com/apache/logging-log4j2/commit/bee90521bb63e6162a9cbed606adde4242675c62].
> Fixing that leak (which actually kept the callback alive) now makes the
> callback completely unreferenced (strongly).
> Suggested fix:
> * make RegisteredCancellable.hook a strong reference
> * make DefaultShutdownCallbackRegistry.hooks a Collection<WeakReference<...>>
> This would keep the callback alive as long as the returned registration
> object (the Cancellable) is alive, and the LoggerContext does reference that.
> ----
> Note: Unclear if the above is actually a complete solution. "LoggerContext
> eligible for GC" doesn't seem to imply "LoggerContext contents all flushed to
> disk" - the strong reference from the shutdown hook to the context actually
> seems necessary, to guarantee that any buffers in the context will be flushed
> even if the context itself happens to become unreferenced.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
