test and eliminate additional sources of cyclic garbage (#2063)

from MultiErrorCatcher.__exit__, _multierror.copy_tb, MultiError.filter, CancelScope.__exit__, and NurseryManager.__aexit__ methods. This was nearly impossible to catch until #1864 landed so it wasn't cleaned up in #1805 (or during the live stream: https://www.youtube.com/watch?v=E_jvJVYXUAk).

newsfragments/2063.bugfix.rst

@@ -0,0 +1,3 @@
+Trio now avoids creating cyclic garbage when a `MultiError` is generated and filtered,
+including invisibly within the cancellation system.  This means errors raised
+through nurseries and cancel scopes should result in less GC latency.

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

@@ -528,12 +528,15 @@ def _close(self, exc):
             self._cancel_status = None
         return exc
 
-    @enable_ki_protection
     def __exit__(self, etype, exc, tb):
         # 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.
 
+        # This inlines the enable_ki_protection decorator so we can fix
+        # f_locals *locally* below to avoid reference cycles
+        locals()[LOCALS_KEY_KI_PROTECTION_ENABLED] = True
+
         # Tracebacks show the 'raise' line below out of context, so let's give
         # this variable a name that makes sense out of context.
         remaining_error_after_cancel_scope = self._close(exc)
@@ -551,6 +554,13 @@ 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
+                # 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
 
     def __repr__(self):
         if self._cancel_status is not None:
@@ -817,6 +827,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"  # pragma: no cover
+
+    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,66 @@ 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
+    def toggle_collected():
+        nonlocal collected
+        collected = True
+
+    collected = False
+    gc.collect()
+    old_flags = gc.get_debug()
+    try:
+        gc.set_debug(0)
+        gc.collect()
+        gc.set_debug(gc.DEBUG_SAVEALL)
+
+        # cover Nursery._nested_child_finished
+        async with _core.open_nursery() as nursery:
+            nursery.cancel_scope.cancel()
+
+        weakref.finalize(nursery, toggle_collected)
+        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()
+
+        assert collected
+        gc.collect()
+        assert not gc.garbage
     finally:
         gc.set_debug(old_flags)
         gc.garbage.clear()