Raju Gupta created LANG-1839:
--------------------------------
Summary: ToStringStyle.VISITED ThreadLocal grows without bound
when a ReflectionToStringBuilder is created but toString() is never called
(memory leak since 3.21.0)
Key: LANG-1839
URL: https://issues.apache.org/jira/browse/LANG-1839
Project: Commons Lang
Issue Type: Bug
Components: lang.builder.*
Affects Versions: 3.21.0, 3.22.0
Environment: Apache Commons Lang 3.21.0 (also reproduced on master
3.22.0-SNAPSHOT, commit 61837eb), OpenJDK 17, macOS
Reporter: Raju Gupta
Since 3.21.0, \{{ToStringStyle.VISITED}} (a ThreadLocal \{{IdentityHashMap}})
can grow without bound and cause an \{{OutOfMemoryError}}. It happens when a
\{{ReflectionToStringBuilder}} is constructed but \{{toString()}} is never
called on it. Reported by Oli Gillespie on the user list ("Memory leak in
ReflectionToStringBuilder/ToStringStyle"). I reproduced it on current master
(3.22.0-SNAPSHOT).
h3. Cause
* Constructing a \{{ToStringBuilder}} calls \{{style.appendStart(...)}}, which
calls \{{register(object)}}. The root object is added to the thread-local
\{{REGISTRY}} in the constructor.
* The root is only removed by \{{unregister(object)}}, called from
\{{appendEnd}} (i.e. \{{toString()}}). A builder that is never stringified
leaves its root in \{{REGISTRY}} for the life of the thread.
* \{{VISITED}} is only cleared in \{{unregister()}}, and only when
\{{REGISTRY.isEmpty()}}.
* \{{RecursiveToStringStyle.appendInternal}} calls \{{markVisited(value)}} for
every object it renders.
Once one orphaned builder exists on a thread, \{{REGISTRY}} is never empty
again, so \{{VISITED}} is never cleared. Every later \{{toString()}} on that
thread adds its whole object graph to \{{VISITED}}, and none of it is released.
The stale \{{REGISTRY}} entry was already possible before 3.21.0, but it cost
one small entry per thread. \{{VISITED}} ties the cleanup of a large per-call
set to that set being empty, which turns the orphan into unbounded growth.
h3. Real-world trigger
{code:java}
log.debug("...", new ReflectionToStringBuilder(...));
{code}
The builder is created eagerly. If debug logging is disabled, \{{toString()}}
is never called.
h3. Reproducer
{code:java}
import org.apache.commons.lang3.builder.*;
import java.lang.reflect.Field;
import java.util.*;
public class VisitedLeak {
static class Graph \{ List<Object> items = new ArrayList<>() {{ for (int i
= 0; i < 999; i++) add(new Object()); }}; }
static int size(String field) throws Exception {
Field f = ToStringStyle.class.getDeclaredField(field);
f.setAccessible(true);
return ((Map<?, ?>) ((ThreadLocal<?>) f.get(null)).get()).size();
}
public static void main(String[] args) throws Exception {
if (args.length == 0) new ReflectionToStringBuilder(new Graph(), new
RecursiveToStringStyle()); // start leak
for (int i = 0; i <= 1000; i++) {
if (i % 100 == 0) System.out.printf("Invocation %d:
REGISTRY.size=%d VISITED.size=%d%n", i, size("REGISTRY"), size("VISITED"));
new ReflectionToStringBuilder(new Graph(), new
RecursiveToStringStyle()).toString();
}
}
}
{code}
Run without arguments (leaks) and with \{{clean}} (control, no orphan builder).
h3. Actual result
* Leak run: \{{Invocation 1000: REGISTRY.size=1 VISITED.size=1000000}}
* Control run: \{{REGISTRY.size=0 VISITED.size=0}}
* With \{{-Xmx64m}} the leak run fails with \{{java.lang.OutOfMemoryError: Java
heap space}} in under 2,000 iterations.
h3. Expected result
{\{VISITED}} is empty after every top-level \{{toString()}} completes,
regardless of builders that were created but never stringified.
h3. Suggested fix
Clear \{{VISITED}} when the outermost \{{ReflectionToStringBuilder.toString()}}
completes (in a \{{finally}}), instead of relying on \{{REGISTRY}} becoming
empty. A nested call always starts after the nested value was marked visited,
so an empty \{{VISITED}} on entry identifies the top-level call and no new
state is needed. I have a patch and a regression test and can open a GitHub PR.
h3. Workaround
Don't construct the builder unless \{{toString()}} will be called, e.g. guard
with \{{if (log.isDebugEnabled())}} or pass a lazy supplier.
--
This message was sent by Atlassian Jira
(v10.3.26#10030026)
