test and eliminate additional sources of cyclic garbage

trio/_core/_ki.py

@@ -156,7 +156,16 @@ def wrapper(*args, **kwargs):
             @wraps(fn)
             def wrapper(*args, **kwargs):
                 locals()[LOCALS_KEY_KI_PROTECTION_ENABLED] = enabled
-                return fn(*args, **kwargs)
+                try:
+                    return fn(*args, **kwargs)
+                finally:
+                    # don't create a refcycle if an exception is passed through and
+                    # re-raised, see test_cancel_scope_exit_doesnt_create_cyclic_garbage
+                    del args, kwargs
+                    # deep magic to remove refs via f_locals
+                    locals()
+                    # TODO: check if PEP558 changes the need for this call
+                    # https://github.com/python/cpython/pull/3640
 
             return wrapper
 

trio/_core/_multierror.py

@@ -153,6 +153,9 @@ def __exit__(self, etype, exc, tb):
                 _, value, _ = sys.exc_info()
                 assert value is filtered_exc
                 value.__context__ = old_context
+                # delete references from locals to avoid creating cycles
+                # see test_MultiError_catch_doesnt_create_cyclic_garbage
+                del _, filtered_exc, value
 
 
 class MultiError(BaseException):
@@ -343,7 +346,12 @@ def copy_tb(base_tb, tb_next):
         c_new_tb.tb_lasti = base_tb.tb_lasti
         c_new_tb.tb_lineno = base_tb.tb_lineno
 
-        return new_tb
+        try:
+            return new_tb
+        finally:
+            # delete references from locals to avoid creating cycles
+            # see test_MultiError_catch_doesnt_create_cyclic_garbage
+            del new_tb, old_tb_frame
 
 
 def concat_tb(head, tail):

trio/_core/_run.py

@@ -530,6 +530,7 @@ def _close(self, exc):
 
     @enable_ki_protection
     def __exit__(self, etype, exc, tb):
+        # locals()["@TRIO_KI_PROTECTION_ENABLED"]=True
         # NB: NurseryManager calls _close() directly rather than __exit__(),
         # so __exit__() must be just _close() plus this logic for adapting
         # the exception-filtering result to the context manager API.
@@ -551,6 +552,9 @@ def __exit__(self, etype, exc, tb):
                 _, value, _ = sys.exc_info()
                 assert value is remaining_error_after_cancel_scope
                 value.__context__ = old_context
+                # delete references from locals to avoid creating cycles
+                # see test_cancel_scope_exit_doesnt_create_cyclic_garbage
+                del remaining_error_after_cancel_scope, value, _, exc
 
     def __repr__(self):
         if self._cancel_status is not None:
@@ -817,6 +821,9 @@ async def __aexit__(self, etype, exc, tb):
                 _, value, _ = sys.exc_info()
                 assert value is combined_error_from_nursery
                 value.__context__ = old_context
+                # delete references from locals to avoid creating cycles
+                # see test_simple_cancel_scope_usage_doesnt_create_cyclic_garbage
+                del _, combined_error_from_nursery, value, new_exc
 
     def __enter__(self):
         raise RuntimeError(

trio/_core/tests/test_multierror.py

@@ -1,3 +1,4 @@
+import gc
 import logging
 import pytest
 
@@ -369,6 +370,38 @@ def catch_RuntimeError(exc):
         assert excinfo.value.__suppress_context__ == suppress_context
 
 
+@pytest.mark.skipif(
+    sys.implementation.name != "cpython", reason="Only makes sense with refcounting GC"
+)
+def test_MultiError_catch_doesnt_create_cyclic_garbage():
+    # https://github.com/python-trio/trio/pull/2063
+    gc.collect()
+    old_flags = gc.get_debug()
+
+    def make_multi():
+        # make_tree creates cycles itself, so a simple
+        raise MultiError([get_exc(raiser1), get_exc(raiser2)])
+
+    def simple_filter(exc):
+        if isinstance(exc, ValueError):
+            return Exception()
+        if isinstance(exc, KeyError):
+            return RuntimeError()
+        assert False, "only ValueError and KeyError should exist"
+
+    try:
+        gc.set_debug(gc.DEBUG_SAVEALL)
+        with pytest.raises(MultiError):
+            # covers MultiErrorCatcher.__exit__ and _multierror.copy_tb
+            with MultiError.catch(simple_filter):
+                raise make_multi()
+        gc.collect()
+        assert not gc.garbage
+    finally:
+        gc.set_debug(old_flags)
+        gc.garbage.clear()
+
+
 def assert_match_in_seq(pattern_list, string):
     offset = 0
     print("looking for pattern matches...")

trio/_core/tests/test_run.py

@@ -2214,16 +2214,25 @@ async def do_a_cancel():
             cscope.cancel()
             await sleep_forever()
 
+    async def crasher():
+        raise ValueError
+
     old_flags = gc.get_debug()
     try:
         gc.collect()
         gc.set_debug(gc.DEBUG_SAVEALL)
 
+        # cover outcome.Error.unwrap
+        # (See https://github.com/python-trio/outcome/pull/29)
         await do_a_cancel()
+        # cover outcome.Error.unwrap if unrolled_run hangs on to exception refs
+        # (See https://github.com/python-trio/trio/pull/1864)
         await do_a_cancel()
 
-        async with _core.open_nursery() as nursery:
-            nursery.start_soon(do_a_cancel)
+        with pytest.raises(ValueError):
+            async with _core.open_nursery() as nursery:
+                # cover MultiError.filter and NurseryManager.__aexit__
+                nursery.start_soon(crasher)
 
         gc.collect()
         assert not gc.garbage
@@ -2235,21 +2244,59 @@ async def do_a_cancel():
 @pytest.mark.skipif(
     sys.implementation.name != "cpython", reason="Only makes sense with refcounting GC"
 )
-async def test_nursery_cancel_doesnt_create_cyclic_garbage():
-    # https://github.com/python-trio/trio/issues/1770#issuecomment-730229423
+async def test_cancel_scope_exit_doesnt_create_cyclic_garbage():
+    # https://github.com/python-trio/trio/pull/2063
     gc.collect()
 
+    async def crasher():
+        raise ValueError
+
     old_flags = gc.get_debug()
     try:
-        for i in range(3):
+
+        with pytest.raises(ValueError), _core.CancelScope() as outer:
             async with _core.open_nursery() as nursery:
                 gc.collect()
-                gc.set_debug(gc.DEBUG_LEAK)
-                nursery.cancel_scope.cancel()
+                gc.set_debug(gc.DEBUG_SAVEALL)
+                # One child that gets cancelled by the outer scope
+                nursery.start_soon(sleep_forever)
+                outer.cancel()
+                # And one that raises a different error
+                nursery.start_soon(crasher)
+                # so that outer filters a Cancelled from the MultiError and
+                # covers CancelScope.__exit__ (and NurseryManager.__aexit__)
+                # (See https://github.com/python-trio/trio/pull/2063)
+
+        gc.collect()
+        assert not gc.garbage
+    finally:
+        gc.set_debug(old_flags)
+        gc.garbage.clear()
 
-            gc.collect()
-            gc.set_debug(0)
-            assert not gc.garbage
+
+@pytest.mark.skipif(
+    sys.implementation.name != "cpython", reason="Only makes sense with refcounting GC"
+)
+async def test_nursery_cancel_doesnt_create_cyclic_garbage():
+    # https://github.com/python-trio/trio/issues/1770#issuecomment-730229423
+    gc.collect()
+
+    old_flags = gc.get_debug()
+    try:
+        gc.set_debug(0)
+        gc.collect()
+        gc.set_debug(gc.DEBUG_SAVEALL)
+
+        async with _core.open_nursery() as nursery:
+            nursery.cancel_scope.cancel()
+
+        del nursery
+        # a checkpoint clears the nursery from the internals, apparently
+        # TODO: stop event loop from hanging on to the nursery at this point
+        await _core.checkpoint()
+
+        gc.collect()
+        assert not gc.garbage
     finally:
         gc.set_debug(old_flags)
         gc.garbage.clear()