Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid creating reference cycles #1805

Merged
merged 3 commits into from
Jan 8, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions newsfragments/1770.bugfix.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
Trio now avoids creating cyclic garbage as often. This should have a
minimal impact on most programs, but can slightly reduce how often the
cycle collector GC runs on CPython, which can reduce latency spikes.
4 changes: 4 additions & 0 deletions trio/_core/_multierror.py
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,9 @@ def push_tb_down(tb, exc, preserved):
preserved = set()
new_root_exc = filter_tree(root_exc, preserved)
push_tb_down(None, root_exc, preserved)
# Delete the local functions to avoid a reference cycle (see
# test_simple_cancel_scope_usage_doesnt_create_cyclic_garbage)
belm0 marked this conversation as resolved.
Show resolved Hide resolved
del filter_tree, push_tb_down
return new_root_exc


Expand All @@ -132,6 +135,7 @@ def __enter__(self):
def __exit__(self, etype, exc, tb):
if exc is not None:
filtered_exc = MultiError.filter(self._handler, exc)

if filtered_exc is exc:
# Let the interpreter re-raise it
return False
Expand Down
7 changes: 6 additions & 1 deletion trio/_core/_run.py
Original file line number Diff line number Diff line change
Expand Up @@ -937,7 +937,12 @@ def aborted(raise_cancel):
popped = self._parent_task._child_nurseries.pop()
assert popped is self
if self._pending_excs:
return MultiError(self._pending_excs)
try:
return MultiError(self._pending_excs)
finally:
# avoid a garbage cycle
# (see test_nursery_cancel_doesnt_create_cyclic_garbage)
del self._pending_excs

def start_soon(self, async_fn, *args, name=None):
"""Creates a child task, scheduling ``await async_fn(*args)``.
Expand Down
54 changes: 54 additions & 0 deletions trio/_core/tests/test_run.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
from contextlib import contextmanager, ExitStack
from math import inf
from textwrap import dedent
import gc

import attr
import outcome
Expand Down Expand Up @@ -2195,3 +2196,56 @@ async def test_cancel_scope_deadline_duplicates():
cscope.deadline = now + 9998
cscope.deadline = now + 9999
await sleep(0.01)


@pytest.mark.skipif(
sys.implementation.name != "cpython", reason="Only makes sense with refcounting GC"
)
async def test_simple_cancel_scope_usage_doesnt_create_cyclic_garbage():
Copy link
Member

@belm0 belm0 Nov 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Would it be worth adding a test to catch garbage regressions at a higher level, say sleep()? Even if that case isn't at 0 now, we could assert on the count being below what's observed currently.

# https://github.com/python-trio/trio/issues/1770
gc.collect()

async def do_a_cancel():
with _core.CancelScope() as cscope:
cscope.cancel()
await sleep_forever()

old_flags = gc.get_debug()
try:
gc.collect()
gc.set_debug(gc.DEBUG_SAVEALL)

await do_a_cancel()
await do_a_cancel()

async with _core.open_nursery() as nursery:
nursery.start_soon(do_a_cancel)

gc.collect()
assert not gc.garbage
finally:
gc.set_debug(old_flags)
gc.garbage.clear()


@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:
for i in range(3):
async with _core.open_nursery() as nursery:
gc.collect()
gc.set_debug(gc.DEBUG_LEAK)
nursery.cancel_scope.cancel()

gc.collect()
gc.set_debug(0)
assert not gc.garbage
finally:
gc.set_debug(old_flags)
gc.garbage.clear()