From 9f039991785aca4e658f370f09a7e8f07873de8b Mon Sep 17 00:00:00 2001 From: Joshua Oreman Date: Sun, 3 Feb 2019 16:51:11 -0800 Subject: [PATCH 1/3] Delay deciding which cancel scope a Cancelled exception belongs to --- newsfragments/860.bugfix.rst | 4 ++ trio/_core/_exceptions.py | 11 +++-- trio/_core/_run.py | 52 +++++++------------- trio/_core/tests/test_run.py | 16 +++--- trio/tests/test_highlevel_open_tcp_stream.py | 8 +-- 5 files changed, 42 insertions(+), 49 deletions(-) create mode 100644 newsfragments/860.bugfix.rst diff --git a/newsfragments/860.bugfix.rst b/newsfragments/860.bugfix.rst new file mode 100644 index 000000000..a0af6d9d8 --- /dev/null +++ b/newsfragments/860.bugfix.rst @@ -0,0 +1,4 @@ +:exc:`trio.Cancelled` exceptions now always propagate until they reach +the outermost unshielded cancelled scope, even if more cancellations +occur or shielding is changed between when the :exc:`~trio.Cancelled` +is delivered and when it is caught. diff --git a/trio/_core/_exceptions.py b/trio/_core/_exceptions.py index 937da2328..0e2ecf38d 100644 --- a/trio/_core/_exceptions.py +++ b/trio/_core/_exceptions.py @@ -47,10 +47,12 @@ class Cancelled(BaseException): then this *won't* catch a :exc:`Cancelled` exception. Attempting to raise :exc:`Cancelled` yourself will cause a - :exc:`RuntimeError`. It would not be associated with a cancel scope and thus - not be caught by Trio. Use - :meth:`cancel_scope.cancel() ` - instead. + :exc:`RuntimeError`. Unless raised within the context of a + cancelled cancel scope, it would not be caught by Trio and would + crash your program. Use :meth:`cancel_scope.cancel() + ` instead, so Trio can maintain the + invariant that a :exc:`Cancelled` exception only gets raised when + there's a cancelled cancel scope that's intended to catch it. .. note:: @@ -65,7 +67,6 @@ class Cancelled(BaseException): everywhere. """ - _scope = None __marker = object() def __init__(self, _marker=None): diff --git a/trio/_core/_run.py b/trio/_core/_run.py index 98b44039b..e297cc555 100644 --- a/trio/_core/_run.py +++ b/trio/_core/_run.py @@ -342,15 +342,6 @@ def shield(self, new_value): for task in self._tasks: task._attempt_delivery_of_any_pending_cancel() - def _cancel_no_notify(self): - # returns the affected tasks - if not self.cancel_called: - with self._might_change_effective_deadline(): - self.cancel_called = True - return self._tasks - else: - return set() - @enable_ki_protection def cancel(self): """Cancels this scope immediately. @@ -358,7 +349,11 @@ def cancel(self): This method is idempotent, i.e., if the scope was already cancelled then this method silently does nothing. """ - for task in self._cancel_no_notify(): + if self.cancel_called: + return + with self._might_change_effective_deadline(): + self.cancel_called = True + for task in self._tasks: task._attempt_delivery_of_any_pending_cancel() def _add_task(self, task): @@ -379,24 +374,23 @@ def _tasks_removed_by_adoption(self, tasks): def _tasks_added_by_adoption(self, tasks): self._tasks.update(tasks) - def _make_exc(self): - exc = Cancelled._init() - exc._scope = self - return exc - def _exc_filter(self, exc): - if isinstance(exc, Cancelled) and exc._scope is self: + if ( + isinstance(exc, Cancelled) + and self.cancel_called + and self._scope_task._pending_cancel_scope() is self + ): self.cancelled_caught = True return None return exc def _close(self, exc): + if exc is not None: + exc = MultiError.filter(self._exc_filter, exc) with self._might_change_effective_deadline(): self._remove_task(self._scope_task) self._scope_task = None - if exc is not None: - return MultiError.filter(self._exc_filter, exc) - return None + return exc @deprecated("0.10.0", issue=607, instead="trio.CancelScope") @@ -432,9 +426,8 @@ def started(self, value=None): # If the old nursery is cancelled, then quietly quit now; the child # will eventually exit on its own, and we don't want to risk moving - # the children into a different scope while they might have - # propagating Cancelled exceptions that assume they're under the old - # scope. + # children that might have propagating Cancelled exceptions into + # a place with no cancelled cancel scopes to catch them. if _pending_cancel_scope(self._old_nursery._cancel_stack) is not None: return @@ -758,13 +751,11 @@ def _attempt_abort(self, raise_cancel): def _attempt_delivery_of_any_pending_cancel(self): if self._abort_func is None: return - pending_scope = self._pending_cancel_scope() - if pending_scope is None: + if self._pending_cancel_scope() is None: return - exc = pending_scope._make_exc() def raise_cancel(): - raise exc + raise Cancelled._init() self._attempt_abort(raise_cancel) @@ -1523,21 +1514,16 @@ def run_impl(runner, async_fn, args): if runner.instruments: runner.instrument("after_io_wait", timeout) + # Process cancellations due to deadline expiry now = runner.clock.current_time() - # We process all timeouts in a batch and then notify tasks at the end - # to ensure that if multiple timeouts occur at once, then it's the - # outermost one that gets delivered. - cancelled_tasks = set() while runner.deadlines: (deadline, _), cancel_scope = runner.deadlines.peekitem(0) if deadline <= now: # This removes the given scope from runner.deadlines: - cancelled_tasks.update(cancel_scope._cancel_no_notify()) + cancel_scope.cancel() idle_primed = False else: break - for task in cancelled_tasks: - task._attempt_delivery_of_any_pending_cancel() if not runner.runq and idle_primed: while runner.waiting_for_idle: diff --git a/trio/_core/tests/test_run.py b/trio/_core/tests/test_run.py index c1d2aa0b8..e03f6da03 100644 --- a/trio/_core/tests/test_run.py +++ b/trio/_core/tests/test_run.py @@ -637,21 +637,21 @@ async def crasher(): nursery.start_soon(crasher) # t4 # and then our __aexit__ also receives an outer Cancelled except _core.MultiError as multi_exc: - # This is outside the nursery scope but inside the outer - # scope, so the nursery should have absorbed t1 and t2's - # exceptions but t3 and t4 should remain, plus the Cancelled - # from 'outer' - assert len(multi_exc.exceptions) == 3 + # Since the outer scope became cancelled before the + # nursery block exited, all cancellations inside the + # nursery block continue propagating to reach the + # outer scope. + assert len(multi_exc.exceptions) == 5 summary = {} for exc in multi_exc.exceptions: summary.setdefault(type(exc), 0) summary[type(exc)] += 1 - assert summary == {_core.Cancelled: 2, KeyError: 1} + assert summary == {_core.Cancelled: 4, KeyError: 1} raise except AssertionError: # pragma: no cover raise except BaseException as exc: - # This is ouside the outer scope, so the two outer Cancelled + # This is ouside the outer scope, so all the Cancelled # exceptions should have been absorbed, leaving just a regular # KeyError from crasher() assert type(exc) is KeyError @@ -1400,7 +1400,7 @@ async def main(): assert len(record) == 2 assert record[0] == "starting" assert record[1][0] == "run finished" - assert record[1][1] >= 20 + assert record[1][1] >= 19 def test_TrioToken_run_sync_soon_threaded_stress_test(): diff --git a/trio/tests/test_highlevel_open_tcp_stream.py b/trio/tests/test_highlevel_open_tcp_stream.py index 9f7d5d88c..35a5619e4 100644 --- a/trio/tests/test_highlevel_open_tcp_stream.py +++ b/trio/tests/test_highlevel_open_tcp_stream.py @@ -397,9 +397,11 @@ async def test_multi_success(autojump_clock): happy_eyeballs_delay=1, ) assert not scenario.sockets["1.1.1.1"].succeeded - assert scenario.sockets["2.2.2.2"].succeeded - assert scenario.sockets["3.3.3.3"].succeeded - assert scenario.sockets["4.4.4.4"].succeeded + assert ( + scenario.sockets["2.2.2.2"].succeeded + or scenario.sockets["3.3.3.3"].succeeded + or scenario.sockets["4.4.4.4"].succeeded + ) assert not scenario.sockets["5.5.5.5"].succeeded assert sock.ip in ["2.2.2.2", "3.3.3.3", "4.4.4.4"] assert trio.current_time() == (0.5 + 10) From 018afc13bb9fbc57ed6f97bdad9bb33cbb8265a6 Mon Sep 17 00:00:00 2001 From: Joshua Oreman Date: Sun, 3 Feb 2019 17:26:49 -0800 Subject: [PATCH 2/3] yapf --- trio/_core/_run.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/trio/_core/_run.py b/trio/_core/_run.py index e297cc555..e00295a1c 100644 --- a/trio/_core/_run.py +++ b/trio/_core/_run.py @@ -376,8 +376,7 @@ def _tasks_added_by_adoption(self, tasks): def _exc_filter(self, exc): if ( - isinstance(exc, Cancelled) - and self.cancel_called + isinstance(exc, Cancelled) and self.cancel_called and self._scope_task._pending_cancel_scope() is self ): self.cancelled_caught = True From f40e10573a1c9d7eec85ab1c310ec22464eacbe4 Mon Sep 17 00:00:00 2001 From: Joshua Oreman Date: Sun, 3 Feb 2019 17:34:27 -0800 Subject: [PATCH 3/3] simplify docs --- trio/_core/_exceptions.py | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/trio/_core/_exceptions.py b/trio/_core/_exceptions.py index 0e2ecf38d..6b4302e39 100644 --- a/trio/_core/_exceptions.py +++ b/trio/_core/_exceptions.py @@ -46,13 +46,9 @@ class Cancelled(BaseException): then this *won't* catch a :exc:`Cancelled` exception. - Attempting to raise :exc:`Cancelled` yourself will cause a - :exc:`RuntimeError`. Unless raised within the context of a - cancelled cancel scope, it would not be caught by Trio and would - crash your program. Use :meth:`cancel_scope.cancel() - ` instead, so Trio can maintain the - invariant that a :exc:`Cancelled` exception only gets raised when - there's a cancelled cancel scope that's intended to catch it. + You cannot raise :exc:`Cancelled` yourself. Attempting to do so + will produce a :exc:`RuntimeError`. Use :meth:`cancel_scope.cancel() + ` instead. .. note::