Skip to content

Commit 44208a2

Browse files
committed
Merge pull request #7 from scijava/remove-leaks
Fix Jython memory leak
2 parents 9e81c53 + 98fa041 commit 44208a2

File tree

3 files changed

+235
-6
lines changed

3 files changed

+235
-6
lines changed

src/main/java/org/scijava/plugins/scripting/jython/JythonBindings.java

Lines changed: 41 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -31,8 +31,10 @@
3131

3232
package org.scijava.plugins.scripting.jython;
3333

34+
import java.lang.ref.WeakReference;
3435
import java.util.ArrayList;
3536
import java.util.Collection;
37+
import java.util.HashMap;
3638
import java.util.HashSet;
3739
import java.util.List;
3840
import java.util.Map;
@@ -42,6 +44,7 @@
4244

4345
import org.python.core.PyStringMap;
4446
import org.python.util.PythonInterpreter;
47+
import org.scijava.script.ScriptModule;
4548

4649
/**
4750
* A {@link Bindings} wrapper around Jython's local variables.
@@ -52,6 +55,25 @@ public class JythonBindings implements Bindings {
5255

5356
protected final PythonInterpreter interpreter;
5457

58+
/*
59+
* NB: In our JythonScriptLanguage we explain the need for cleaning
60+
* up after a PythonInterpreter and declare the scope of a
61+
* Python environment to be equal to the lifetime of its parent
62+
* JythonScriptEngine.
63+
* As triggering our cleaning method involves PhantomReferences
64+
* we must ensure that JythonScriptEngines can actually be
65+
* garbage collected when it is no longer in use.
66+
* To do this, we have to prevent JythonScriptEngines from
67+
* being passed to the PythonInterpreter - which would then
68+
* create a hard reference to the ScriptEngine through the
69+
* static PySystemState.
70+
* Similarly we do not want to pass ScriptModules to the
71+
* PythonInterpreter, as the ScriptModule has a hard
72+
* reference to its ScriptEngine.
73+
*/
74+
private Map<String, WeakReference<Object>> shallowMap =
75+
new HashMap<String, WeakReference<Object>>();
76+
5577
public JythonBindings(final PythonInterpreter interpreter) {
5678
this.interpreter = interpreter;
5779
}
@@ -81,6 +103,10 @@ public boolean containsValue(Object value) {
81103

82104
@Override
83105
public Object get(Object key) {
106+
if (shallowMap.containsKey(key)) {
107+
return shallowMap.get(key).get();
108+
}
109+
84110
try {
85111
return interpreter.get((String)key);
86112
} catch (Error e) {
@@ -91,18 +117,28 @@ public Object get(Object key) {
91117
@Override
92118
public Object put(String key, Object value) {
93119
final Object result = get(key);
94-
try {
95-
interpreter.set(key, value);
96-
} catch (Error e) {
97-
// ignore
120+
121+
if (value instanceof ScriptModule || value instanceof JythonScriptEngine){
122+
shallowMap.put(key, new WeakReference<Object>(value));
123+
}
124+
else {
125+
try {
126+
interpreter.set(key, value);
127+
}
128+
catch (Error e) {
129+
// ignore
130+
}
98131
}
132+
99133
return result;
100134
}
101135

102136
@Override
103137
public Object remove(Object key) {
104138
final Object result = get(key);
105-
if (result != null) interpreter.getLocals().__delitem__((String)key);
139+
if (shallowMap.containsKey(key)) shallowMap.remove(key);
140+
else if (result != null) interpreter.getLocals().__delitem__((String)key);
141+
106142
return result;
107143
}
108144

Lines changed: 179 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,179 @@
1+
/*
2+
* #%L
3+
* JSR-223-compliant Jython scripting language plugin.
4+
* %%
5+
* Copyright (C) 2008 - 2015 Board of Regents of the University of
6+
* Wisconsin-Madison, Broad Institute of MIT and Harvard, and Max Planck
7+
* Institute of Molecular Cell Biology and Genetics.
8+
* %%
9+
* Redistribution and use in source and binary forms, with or without
10+
* modification, are permitted provided that the following conditions are met:
11+
*
12+
* 1. Redistributions of source code must retain the above copyright notice,
13+
* this list of conditions and the following disclaimer.
14+
* 2. Redistributions in binary form must reproduce the above copyright notice,
15+
* this list of conditions and the following disclaimer in the documentation
16+
* and/or other materials provided with the distribution.
17+
*
18+
* THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
19+
* AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
20+
* IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
21+
* ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDERS OR CONTRIBUTORS BE
22+
* LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
23+
* CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
24+
* SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
25+
* INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
26+
* CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
27+
* ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
28+
* POSSIBILITY OF SUCH DAMAGE.
29+
* #L%
30+
*/
31+
32+
package org.scijava.plugins.scripting.jython;
33+
34+
import java.lang.ref.PhantomReference;
35+
import java.lang.ref.ReferenceQueue;
36+
import java.util.ArrayList;
37+
import java.util.LinkedList;
38+
import java.util.List;
39+
40+
import org.python.core.PyObject;
41+
import org.python.util.PythonInterpreter;
42+
import org.scijava.log.LogService;
43+
import org.scijava.thread.ThreadService;
44+
45+
/**
46+
* A helper class for purging dangling references within a Jython interpreter
47+
* after it executes a script.
48+
*
49+
* @author Mark Hiner
50+
*/
51+
public class JythonReferenceCleaner {
52+
53+
// List of PhantomReferences and corresponding ReferenceQueue to
54+
// facilitate proper PhantomReference use.
55+
// See http://resources.ej-technologies.com/jprofiler/help/doc/index.html
56+
57+
private final LinkedList<JythonEnginePhantomReference> phantomReferences =
58+
new LinkedList<JythonEnginePhantomReference>();
59+
private final ReferenceQueue<JythonScriptEngine> queue =
60+
new ReferenceQueue<JythonScriptEngine>();
61+
62+
/** Queues the future cleanup operation on a separate thread, */
63+
public synchronized void queueCleanup(final JythonScriptEngine engine,
64+
final ThreadService threadService, final LogService log)
65+
{
66+
// NB: This phantom reference is used to clean up any local variables
67+
// created by evaluation of scripts via this ScriptEngine. We need
68+
// to use PhantomReferences because the "scope" of a script extends
69+
// beyond its eval method - a consumer may still want to inspect
70+
// the state of variables after evaluation.
71+
//
72+
// By using PhantomReferences we are saying that the scope of
73+
// script evaluation equals the lifetime of the ScriptEngine.
74+
phantomReferences.add(new JythonEnginePhantomReference(engine, queue));
75+
76+
// NB: The use of PhantomReferences requires a paired polling thread.
77+
// We poll instead of blocking for input to avoid leaving lingering
78+
// threads that would need to be interrupted - instead only starting
79+
// threads running when there is actually a PhantomReference that
80+
// will eventually be enqueued.
81+
//
82+
// Here we check if there is already a polling thread in operation -
83+
// if not, we start a new thread.
84+
if (phantomReferences.size() == 1) {
85+
threadService.run(new Runnable() {
86+
87+
@Override
88+
public void run() {
89+
boolean done = false;
90+
91+
// poll the queue
92+
while (!done) {
93+
try {
94+
Thread.sleep(100);
95+
96+
synchronized (JythonReferenceCleaner.this) {
97+
// poll the queue
98+
final JythonEnginePhantomReference ref =
99+
(JythonEnginePhantomReference) queue.poll();
100+
101+
// if we have a ref, clean it up
102+
if (ref != null) {
103+
ref.cleanup();
104+
phantomReferences.remove(ref);
105+
}
106+
107+
// Once we're done with our known phantom refs
108+
// we can let this thread shut down
109+
done = phantomReferences.size() == 0;
110+
}
111+
112+
}
113+
catch (final Exception ex) {
114+
// log exception, continue
115+
log.error(ex);
116+
}
117+
}
118+
}
119+
});
120+
}
121+
}
122+
123+
// -- Helper classes --
124+
125+
/**
126+
* Helper class to clean up {@link PythonInterpreter} local variables when a
127+
* parent {@link JythonScriptEngine} leaves scope.
128+
*/
129+
private static class JythonEnginePhantomReference extends
130+
PhantomReference<JythonScriptEngine>
131+
{
132+
133+
public PythonInterpreter interpreter;
134+
135+
public JythonEnginePhantomReference(final JythonScriptEngine engine,
136+
final ReferenceQueue<JythonScriptEngine> queue)
137+
{
138+
super(engine, queue);
139+
interpreter = engine.interpreter;
140+
}
141+
142+
public void cleanup() {
143+
final List<String> scriptLocals = new ArrayList<String>();
144+
final PythonInterpreter interp = interpreter;
145+
if (interp == null) return;
146+
147+
// NB: This method for cleaning up local variables was taken from:
148+
// http://python.6.x6.nabble.com/Cleaning-up-PythonInterpreter-object-td1777184.html
149+
//
150+
// Because Python is an interpreted language, when a Python script
151+
// creates new variables they stick around in a static
152+
// org.python.core.PySystemState variable (defaultSystemState)
153+
// the org.python.core.Py class.
154+
//
155+
// Thus an implicit static state is created by script evaluation,
156+
// so we must manually clean up local variables known to the
157+
// interpreter when the scope of an executed script is over.
158+
//
159+
// See original bug report for the leak that prompted this solution:
160+
// http://fiji.sc/bugzilla/show_bug.cgi?id=1203
161+
162+
final PyObject locals = interp.getLocals();
163+
for (final PyObject item : locals.__iter__().asIterable()) {
164+
final String localVar = item.toString();
165+
// Ignore __name__ and __doc__ variables, which are special
166+
// and known not to be local variables created by evaluation.
167+
if (!localVar.contains("__name__") && !localVar.contains("__doc__")) {
168+
// Build list of variables to clean
169+
scriptLocals.add(item.toString());
170+
}
171+
}
172+
// Null out local variables
173+
for (final String string : scriptLocals) {
174+
interp.set(string, null);
175+
}
176+
}
177+
}
178+
179+
}

src/main/java/org/scijava/plugins/scripting/jython/JythonScriptLanguage.java

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -36,27 +36,41 @@
3636
import org.python.core.PyNone;
3737
import org.python.core.PyObject;
3838
import org.python.core.PyString;
39+
import org.scijava.log.LogService;
40+
import org.scijava.plugin.Parameter;
3941
import org.scijava.plugin.Plugin;
4042
import org.scijava.script.AdaptedScriptLanguage;
4143
import org.scijava.script.ScriptLanguage;
44+
import org.scijava.thread.ThreadService;
4245

4346
/**
4447
* An adapter of the Jython interpreter to the SciJava scripting interface.
4548
*
4649
* @author Johannes Schindelin
50+
* @author Mark Hiner <hinerm@gmail.com>
4751
* @see ScriptEngine
4852
*/
4953
@Plugin(type = ScriptLanguage.class, name = "Python")
5054
public class JythonScriptLanguage extends AdaptedScriptLanguage {
5155

56+
private JythonReferenceCleaner cleaner = new JythonReferenceCleaner();
57+
58+
@Parameter
59+
private ThreadService threadService;
60+
61+
@Parameter
62+
private LogService logService;
63+
5264
public JythonScriptLanguage() {
5365
super("jython");
5466
}
5567

5668
@Override
5769
public ScriptEngine getScriptEngine() {
5870
// TODO: Consider adapting the wrapped ScriptEngineFactory's ScriptEngine.
59-
return new JythonScriptEngine();
71+
final JythonScriptEngine engine = new JythonScriptEngine();
72+
cleaner.queueCleanup(engine, threadService, logService);
73+
return engine;
6074
}
6175

6276
@Override

0 commit comments

Comments
 (0)