From 3f4e1a23e567825926b225f87c64db56d95f96fb Mon Sep 17 00:00:00 2001 From: richardsheridan Date: Sun, 18 Jul 2021 19:24:44 -0400 Subject: [PATCH] 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 | 3 ++ trio/_core/_multierror.py | 10 +++- trio/_core/_run.py | 15 +++++- trio/_core/tests/test_multierror.py | 33 +++++++++++++ trio/_core/tests/test_run.py | 74 +++++++++++++++++++++++++---- 5 files changed, 123 insertions(+), 12 deletions(-) create mode 100644 newsfragments/2063.bugfix.rst diff --git a/newsfragments/2063.bugfix.rst b/newsfragments/2063.bugfix.rst new file mode 100644 index 0000000000..85fe7aa52b --- /dev/null +++ b/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. \ No newline at end of file diff --git a/trio/_core/_multierror.py b/trio/_core/_multierror.py index ed926c029f..b6bf2cf727 100644 --- a/trio/_core/_multierror.py +++ b/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): diff --git a/trio/_core/_run.py b/trio/_core/_run.py index 3174261fa9..a40a7a29ae 100644 --- a/trio/_core/_run.py +++ b/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( diff --git a/trio/_core/tests/test_multierror.py b/trio/_core/tests/test_multierror.py index 14eab22df7..70d763e652 100644 --- a/trio/_core/tests/test_multierror.py +++ b/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...") diff --git a/trio/_core/tests/test_run.py b/trio/_core/tests/test_run.py index c56aaaa092..a559ac11ed 100644 --- a/trio/_core/tests/test_run.py +++ b/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()