Work again on bug 269966.

I think this time we do have a robust fix.  I added tests that do fail after
John Hunter's example, so if this one ever tries to return, we should catch
it right away.

Very, very nasty bug...

IPython/tests/refbug.py

@@ -0,0 +1,38 b''
+"""Minimal script to reproduce our nasty reference counting bug.
+
+The problem is related to https://bugs.launchpad.net/ipython/+bug/269966
+
+The original fix for that appeared to work, but JD Hunter found a matplotlib
+example which, when run twice in a row, would break.  The problem were
+references held by open figures to internals of Tkinter.
+
+This code reproduces the problem that John saw, without matplotlib.  We can
+thus use it for our test suite.
+"""
+
+#-----------------------------------------------------------------------------
+# Module imports
+#-----------------------------------------------------------------------------
+import sys
+
+from IPython import ipapi
+
+#-----------------------------------------------------------------------------
+# Globals
+#-----------------------------------------------------------------------------
+ip = ipapi.get()
+
+if not '_refbug_cache' in ip.user_ns:
+    ip.user_ns['_refbug_cache'] = []
+
+
+aglobal = 'Hello'
+def f():
+    return aglobal
+
+cache = ip.user_ns['_refbug_cache']
+cache.append(f)
+
+def call_f():
+    for func in cache:
+        print 'lowercased:',func().lower()

IPython/tests/test_fakemodule.py

@@ -0,0 +1,17 b''
+"""Tests for the FakeModule objects.
+"""
+
+import nose.tools as nt
+
+from IPython.FakeModule import FakeModule, init_fakemod_dict
+
+# Make a fakemod and check a few properties
+def test_mk_fakemod():
+    fm = FakeModule()
+    yield nt.assert_true,fm
+    yield nt.assert_true,lambda : hasattr(fm,'__file__')
+
+def test_mk_fakemod_fromdict():
+    """Test making a FakeModule object with initial data"""
+    fm = FakeModule(dict(hello=True))
+    nt.assert_true(fm.hello)

IPython/FakeModule.py

@@ -15,6 +15,37 b' sessions.'
 
 import types
 
+def init_fakemod_dict(fm,adict=None):
+    """Initialize a FakeModule instance __dict__.
+
+    Kept as a standalone function and not a method so the FakeModule API can
+    remain basically empty.
+
+    This should be considered for private IPython use, used in managing
+    namespaces for %run.
+
+    Parameters
+    ----------
+
+    fm : FakeModule instance
+
+    adict : dict, optional
+    """
+
+    dct = {}
+    # It seems pydoc (and perhaps others) needs any module instance to
+    # implement a __nonzero__ method, so we add it if missing:
+    dct.setdefault('__nonzero__',lambda : True)
+    dct.setdefault('__file__',__file__)
+
+    if adict is not None:
+        dct.update(adict)
+
+    # Hard assignment of the object's __dict__.  This is nasty but deliberate.
+    fm.__dict__.clear()
+    fm.__dict__.update(dct)
+
+
 class FakeModule(types.ModuleType):
     """Simple class with attribute access to fake a module.
 
@@ -29,14 +60,7 b' class FakeModule(types.ModuleType):'
         
         # tmp to force __dict__ instance creation, else self.__dict__ fails
         self.__iptmp = None
-        
-        # It seems pydoc (and perhaps others) needs any module instance to
-        # implement a __nonzero__ method, so we add it if missing:
-        self.__dict__.setdefault('__nonzero__',lambda : True)
-        self.__dict__.setdefault('__file__',__file__)
-
         # cleanup our temp trick
         del self.__iptmp
-
-        if adict is not None:
-            self.__dict__.update(adict)
+        # Now, initialize the actual data in the instance dict.
+        init_fakemod_dict(self,adict)

IPython/Magic.py

@@ -1584,23 +1584,22 b' Currently the magic system has the following functions:\\n"""'
             prog_ns = self.shell.user_ns
             __name__save = self.shell.user_ns['__name__']
             prog_ns['__name__'] = '__main__'
-            main_mod = FakeModule(prog_ns)
+
+            ##main_mod = FakeModule(prog_ns)
+            main_mod = self.shell.new_main_mod(prog_ns)
+            
         else:
             # Run in a fresh, empty namespace
             if opts.has_key('n'):
                 name = os.path.splitext(os.path.basename(filename))[0]
             else:
                 name = '__main__'
-            main_mod = FakeModule()
+
+            main_mod = self.shell.new_main_mod()
+            
             prog_ns = main_mod.__dict__
             prog_ns['__name__'] = name
             
-            # The shell MUST hold a reference to main_mod so after %run exits,
-            # the python deletion mechanism doesn't zero it out (leaving
-            # dangling references).  However, we should drop old versions of
-            # main_mod.  There is now a proper API to manage this caching in
-            # the main shell object, we use that.            
-            self.shell.cache_main_mod(main_mod)
 
         # Since '%run foo' emulates 'python foo.py' at the cmd line, we must
         # set the __file__ global in the script's namespace
@@ -1703,9 +1702,14 b' Currently the magic system has the following functions:\\n"""'
                     else:
                         # regular execution
                         runner(filename,prog_ns,prog_ns,exit_ignore=exit_ignore)
+
                 if opts.has_key('i'):
                     self.shell.user_ns['__name__'] = __name__save
                 else:
+                    # The shell MUST hold a reference to prog_ns so after %run
+                    # exits, the python deletion mechanism doesn't zero it out
+                    # (leaving dangling references).
+                    self.shell.cache_main_mod(prog_ns,filename)
                     # update IPython interactive namespace
                     del prog_ns['__name__']
                     self.shell.user_ns.update(prog_ns)
@@ -1719,6 +1723,7 b' Currently the magic system has the following functions:\\n"""'
                 # added.  Otherwise it will trap references to objects
                 # contained therein.
                 del sys.modules[main_mod_name]
+
             self.shell.reloadhist()
                 
         return stats

IPython/iplib.py

@@ -54,7 +54,7 b' from pprint import pprint, pformat'
 from IPython import Debugger,OInspect,PyColorize,ultraTB
 from IPython.ColorANSI import ColorScheme,ColorSchemeTable  # too long names
 from IPython.Extensions import pickleshare
-from IPython.FakeModule import FakeModule
+from IPython.FakeModule import FakeModule, init_fakemod_dict
 from IPython.Itpl import Itpl,itpl,printpl,ItplNS,itplns
 from IPython.Logger import Logger
 from IPython.Magic import Magic
@@ -499,13 +499,24 b' class InteractiveShell(object,Magic):'
         # calling functions defined in the script that use other things from
         # the script will fail, because the function's closure had references
         # to the original objects, which are now all None.  So we must protect
-        # these modules from deletion by keeping a cache.  To avoid keeping
-        # stale modules around (we only need the one from the last run), we use
-        # a dict keyed with the full path to the script, so only the last
-        # version of the module is held in the cache.  The %reset command will
-        # flush this cache.  See the cache_main_mod() and clear_main_mod_cache()
-        # methods for details on use.
-        self._user_main_modules = {}
+        # these modules from deletion by keeping a cache.
+        # 
+        # To avoid keeping stale modules around (we only need the one from the
+        # last run), we use a dict keyed with the full path to the script, so
+        # only the last version of the module is held in the cache.  Note,
+        # however, that we must cache the module *namespace contents* (their
+        # __dict__).  Because if we try to cache the actual modules, old ones
+        # (uncached) could be destroyed while still holding references (such as
+        # those held by GUI objects that tend to be long-lived)>
+        # 
+        # The %reset command will flush this cache.  See the cache_main_mod()
+        # and clear_main_mod_cache() methods for details on use.
+
+        # This is the cache used for 'main' namespaces
+        self._main_ns_cache = {}
+        # And this is the single instance of FakeModule whose __dict__ we keep
+        # copying and clearing for reuse on each %run
+        self._user_main_module = FakeModule()
 
         # A table holding all the namespaces IPython deals with, so that
         # introspection facilities can search easily.
@@ -521,7 +532,7 b' class InteractiveShell(object,Magic):'
         # a simple list.
         self.ns_refs_table = [ user_ns, user_global_ns, self.user_config_ns,
                                self.alias_table, self.internal_ns,
-                               self._user_main_modules ]
+                               self._main_ns_cache ]
         
         # We need to insert into sys.modules something that looks like a
         # module but which accesses the IPython namespace, for shelve and
@@ -1487,38 +1498,53 b' class InteractiveShell(object,Magic):'
             return True
         return ask_yes_no(prompt,default)
 
-    def cache_main_mod(self,mod,fname=None):
-        """Cache a main module.
+    def new_main_mod(self,ns=None):
+        """Return a new 'main' module object for user code execution.
+        """
+        main_mod = self._user_main_module
+        init_fakemod_dict(main_mod,ns)
+        return main_mod
+
+    def cache_main_mod(self,ns,fname):
+        """Cache a main module's namespace.
 
-        When scripts are executed via %run, we must keep a reference to their
-        __main__ module (a FakeModule instance) around so that Python doesn't
-        clear it, rendering objects defined therein useless.
+        When scripts are executed via %run, we must keep a reference to the
+        namespace of their __main__ module (a FakeModule instance) around so
+        that Python doesn't clear it, rendering objects defined therein
+        useless.
 
         This method keeps said reference in a private dict, keyed by the
         absolute path of the module object (which corresponds to the script
         path).  This way, for multiple executions of the same script we only
-        keep one copy of __main__ (the last one), thus preventing memory leaks
-        from old references while allowing the objects from the last execution
-        to be accessible.
+        keep one copy of the namespace (the last one), thus preventing memory
+        leaks from old references while allowing the objects from the last
+        execution to be accessible.
+
+        Note: we can not allow the actual FakeModule instances to be deleted,
+        because of how Python tears down modules (it hard-sets all their
+        references to None without regard for reference counts).  This method
+        must therefore make a *copy* of the given namespace, to allow the
+        original module's __dict__ to be cleared and reused.
 
+        
         Parameters
         ----------
-          mod : a module object
+          ns : a namespace (a dict, typically)
+
+          fname : str
+            Filename associated with the namespace.
 
         Examples
         --------
 
         In [10]: import IPython
 
-        In [11]: _ip.IP.cache_main_mod(IPython)
+        In [11]: _ip.IP.cache_main_mod(IPython.__dict__,IPython.__file__)
 
-        In [12]: IPython.__file__ in _ip.IP._user_main_modules
+        In [12]: IPython.__file__ in _ip.IP._main_ns_cache
         Out[12]: True
         """
-        if fname is None:
-            fname = mod.__file__
-        #print >> sys.stderr, 'CFNAME  :', os.path.abspath(fname)  # dbg
-        self._user_main_modules[os.path.abspath(fname)] = mod
+        self._main_ns_cache[os.path.abspath(fname)] = ns.copy()
 
     def clear_main_mod_cache(self):
         """Clear the cache of main modules.
@@ -1530,17 +1556,17 b' class InteractiveShell(object,Magic):'
 
         In [15]: import IPython
 
-        In [16]: _ip.IP.cache_main_mod(IPython)
+        In [16]: _ip.IP.cache_main_mod(IPython.__dict__,IPython.__file__)
 
-        In [17]: len(_ip.IP._user_main_modules) > 0
+        In [17]: len(_ip.IP._main_ns_cache) > 0
         Out[17]: True
 
         In [18]: _ip.IP.clear_main_mod_cache()
 
-        In [19]: len(_ip.IP._user_main_modules) == 0
+        In [19]: len(_ip.IP._main_ns_cache) == 0
         Out[19]: True
         """
-        self._user_main_modules.clear()
+        self._main_ns_cache.clear()
 
     def _should_recompile(self,e):
         """Utility routine for edit_syntax_error"""

IPython/tests/test_magic.py

@@ -136,6 +136,8 b' def doctest_refbug():'
     """Very nasty problem with references held by multiple runs of a script.
     See: https://bugs.launchpad.net/ipython/+bug/269966
 
+    In [1]: _ip.IP.clear_main_mod_cache()
+    
     In [2]: run refbug
 
     In [3]: call_f()