Document PhantomReference solution

Add extensive commenting to explain why PhantomReferences are needed to
solve the memory leak identified in this implementation of the Python
script language.

Fixes http://fiji.sc/bugzilla/show_bug.cgi?id=1203

Author: hinerm

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

@@ -55,6 +55,22 @@ public class JythonBindings implements Bindings {
 
 	protected final PythonInterpreter interpreter;
 
+	/*
+	 * NB: In our JythonScriptLanguage we explain the need for cleaning
+	 * up after a PythonInterpreter and declare the scope of a
+	 * Python environment to be equal to the lifetime of its parent
+	 * JythonScriptEngine.
+	 * As triggering our cleaning method involves PhantomReferences
+	 * we must ensure that JythonScriptEngines can actually be
+	 * garbage collected when it is no longer in use.
+	 * To do this, we have to prevent JythonScriptEngines from
+	 * being passed to the PythonInterpreter - which would then
+	 * create a hard reference to the ScriptEngine through the
+	 * static PySystemState.
+	 * Similarly we do not want to pass ScriptModules to the
+	 * PythonInterpreter, as the ScriptModule has a hard
+	 * reference to its ScriptEngine.
+	 */
 	private Map<String, WeakReference<Object>> shallowMap =
 		new HashMap<String, WeakReference<Object>>();
 

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

@@ -53,11 +53,16 @@
  * An adapter of the Jython interpreter to the SciJava scripting interface.
  * 
  * @author Johannes Schindelin
+ * @author Mark Hiner <[email protected]>
  * @see ScriptEngine
  */
 @Plugin(type = ScriptLanguage.class, name = "Python")
 public class JythonScriptLanguage extends AdaptedScriptLanguage {
 
+	// List of PhantomReferences and corresponding ReferenceQueue to
+	// facilitate proper PhantomReference use.
+	// See http://resources.ej-technologies.com/jprofiler/help/doc/index.html
+
 	private final LinkedList<JythonEnginePhantomReference> phantomReferences =
 		new LinkedList<JythonEnginePhantomReference>();
 	private final ReferenceQueue<JythonScriptEngine> queue =
@@ -76,10 +81,22 @@ public ScriptEngine getScriptEngine() {
 		final JythonScriptEngine engine = new JythonScriptEngine();
 
 		synchronized (phantomReferences) {
+			// NB: This phantom reference is used to clean up any local variables
+			// created by evaluation of scripts via this ScriptEngine. We need
+			// to use PhantomReferences because the "scope" of a script extends
+			// beyond its eval method - a consumer may still want to inspect
+			// the state of variables after evaluation.
+			// By using PhantomReferences we are saying that the scope of 
+			// script evaluation equals the lifetime of the ScriptEngine.
 			phantomReferences.add(new JythonEnginePhantomReference(engine, queue));
 
-			// If we added references to an empty queue, we need to start
-			// a new polling thread
+			// NB: The use of PhantomReferences requires a paired polling thread.
+			// We poll instead of blocking for input to avoid leaving lingering
+			// threads that would need to be interrupted - instead only starting
+			// threads running when there is actually a PhantomReference that
+			// will eventually be enqueued.
+			// Here we check if there is already a polling thread in operation -
+			// if not, we start a new thread.
 			if (phantomReferences.size() == 1) {
 				threadService.run(new Runnable() {
 
@@ -93,6 +110,7 @@ public void run() {
 								Thread.sleep(100);
 
 								synchronized (phantomReferences) {
+									// poll the queue
 									JythonEnginePhantomReference ref =
 										(JythonEnginePhantomReference) queue.poll();
 
@@ -103,7 +121,7 @@ public void run() {
 									}
 
 									// Once we're done with our known phantom refs
-									// we can shut down this thread.
+									// we can let this thread shut down
 									done = phantomReferences.size() == 0;
 								}
 
@@ -135,6 +153,10 @@ public Object decode(final Object object) {
 		return object;
 	}
 
+	/**
+	 * Helper class to clean up {@link PythonInterpreter} local variables when a
+	 * parent {@link JythonScriptEngine} leaves scope.
+	 */
 	private static class JythonEnginePhantomReference extends
 		PhantomReference<JythonScriptEngine>
 	{
@@ -153,14 +175,27 @@ public void cleanup() {
 			PythonInterpreter interp = interpreter;
 			if (interp == null) return;
 
+			// NB: This method for cleaning up local variables was taken from:
+			// http://python.6.x6.nabble.com/Cleaning-up-PythonInterpreter-object-td1777184.html
+			// Because Python is an interpreted language, when a Python script creates new
+			// variables they stick around in a static org.python.core.PySystemState  variable
+			// (defaultSystemState) the org.python.core.Py class.
+			// Thus an implicit static state is created by script evaluation, so we must manually
+			// clean up local variables known to the interpreter when the scope of an executed
+			// script is over.
+			// See original bug report for the memory leak that prompted this solution:
+			// http://fiji.sc/bugzilla/show_bug.cgi?id=1203
 			final PyObject locals = interp.getLocals();
 			for (final PyObject item : locals.__iter__().asIterable()) {
 				final String localVar = item.toString();
+				// Ignore __name__ and __doc__ variables, which are special and known not
+				// to be local variables created by evaluation.
 				if (!localVar.contains("__name__") && !localVar.contains("__doc__")) {
-
+					// Build list of variables to clean
 					scriptLocals.add(item.toString());
 				}
 			}
+			// Null out local variables
 			for (final String string : scriptLocals) {
 				interp.set(string, null);
 			}