From 2f36eaa880ccc8414054ebef7dd036b5cf19051b Mon Sep 17 00:00:00 2001 From: Joshua Oreman Date: Tue, 19 May 2020 09:52:15 +0000 Subject: [PATCH 1/9] Inject deferred KeyboardInterrupt in a new task, instead of waking up the main task to deliver it --- trio/_core/_exceptions.py | 16 +++++++++ trio/_core/_io_kqueue.py | 4 +-- trio/_core/_io_windows.py | 12 +++---- trio/_core/_run.py | 68 +++++++++++++++++++----------------- trio/_core/_traps.py | 55 ++++++++++++++++------------- trio/_core/tests/test_ki.py | 5 ++- trio/_core/tests/test_run.py | 10 +++--- 7 files changed, 95 insertions(+), 75 deletions(-) diff --git a/trio/_core/_exceptions.py b/trio/_core/_exceptions.py index b481f7c50d..0887c3849a 100644 --- a/trio/_core/_exceptions.py +++ b/trio/_core/_exceptions.py @@ -1,6 +1,9 @@ +# coding: utf-8 + import attr from trio._util import NoPublicConstructor +from trio import _deprecate class TrioInternalError(Exception): @@ -65,6 +68,19 @@ class Cancelled(BaseException, metaclass=NoPublicConstructor): def __str__(self): return "Cancelled" + def __call__(self): + # If a Cancelled exception is passed to an old abort_fn that + # expects a raise_cancel callback, someone will eventually try + # to call the exception instead of raising it. Provide a + # deprecation warning and raise it instead. + _deprecate.warn_deprecated( + "wait_task_rescheduled's abort_fn taking a callback argument", + "0.16.0", + issue=1536, + instead="an exception argument", + ) + raise self + class BusyResourceError(Exception): """Raised when a task attempts to use a resource that some other task is diff --git a/trio/_core/_io_kqueue.py b/trio/_core/_io_kqueue.py index 8f0f492c69..593f2e353a 100644 --- a/trio/_core/_io_kqueue.py +++ b/trio/_core/_io_kqueue.py @@ -104,8 +104,8 @@ async def wait_kevent(self, ident, filter, abort_func): ) self._registered[key] = _core.current_task() - def abort(raise_cancel): - r = abort_func(raise_cancel) + def abort(exc): + r = abort_func(exc) if r is _core.Abort.SUCCEEDED: del self._registered[key] return r diff --git a/trio/_core/_io_windows.py b/trio/_core/_io_windows.py index 7e452177d1..6302094888 100644 --- a/trio/_core/_io_windows.py +++ b/trio/_core/_io_windows.py @@ -619,11 +619,11 @@ async def wait_overlapped(self, handle, lpOverlapped): ) task = _core.current_task() self._overlapped_waiters[lpOverlapped] = task - raise_cancel = None + cancel_exc = None - def abort(raise_cancel_): - nonlocal raise_cancel - raise_cancel = raise_cancel_ + def abort(cancel_exc_): + nonlocal cancel_exc + cancel_exc = cancel_exc_ try: _check(kernel32.CancelIoEx(handle, lpOverlapped)) except OSError as exc: @@ -663,8 +663,8 @@ def abort(raise_cancel_): # it will produce the right sorts of exceptions code = ntdll.RtlNtStatusToDosError(lpOverlapped.Internal) if code == ErrorCodes.ERROR_OPERATION_ABORTED: - if raise_cancel is not None: - raise_cancel() + if cancel_exc is not None: + raise cancel_exc else: # We didn't request this cancellation, so assume # it happened due to the underlying handle being diff --git a/trio/_core/_run.py b/trio/_core/_run.py index 5904e682fd..02a3a22bff 100644 --- a/trio/_core/_run.py +++ b/trio/_core/_run.py @@ -1,3 +1,5 @@ +# coding: utf-8 + import functools import itertools import logging @@ -829,8 +831,8 @@ async def _nested_child_finished(self, nested_child_exc): # If we get cancelled (or have an exception injected, like # KeyboardInterrupt), then save that, but still wait until our # children finish. - def aborted(raise_cancel): - self._add_exc(capture(raise_cancel).error) + def aborted(exc): + self._add_exc(exc) return Abort.FAILED self._parent_waiting_in_aexit = True @@ -1042,41 +1044,26 @@ def _activate_cancel_status(self, cancel_status): if self._cancel_status.effectively_cancelled: self._attempt_delivery_of_any_pending_cancel() - def _attempt_abort(self, raise_cancel): + def _attempt_abort(self, exc): # Either the abort succeeds, in which case we will reschedule the # task, or else it fails, in which case it will worry about - # rescheduling itself (hopefully eventually calling reraise to raise + # rescheduling itself (hopefully eventually raising # the given exception, but not necessarily). - success = self._abort_func(raise_cancel) + success = self._abort_func(exc) if type(success) is not Abort: raise TrioInternalError("abort function must return Abort enum") # We only attempt to abort once per blocking call, regardless of # whether we succeeded or failed. self._abort_func = None if success is Abort.SUCCEEDED: - self._runner.reschedule(self, capture(raise_cancel)) + self._runner.reschedule(self, Error(exc)) def _attempt_delivery_of_any_pending_cancel(self): if self._abort_func is None: return if not self._cancel_status.effectively_cancelled: return - - def raise_cancel(): - raise Cancelled._create() - - self._attempt_abort(raise_cancel) - - def _attempt_delivery_of_pending_ki(self): - assert self._runner.ki_pending - if self._abort_func is None: - return - - def raise_cancel(): - self._runner.ki_pending = False - raise KeyboardInterrupt - - self._attempt_abort(raise_cancel) + self._attempt_abort(Cancelled._create()) ################################################################ @@ -1498,12 +1485,6 @@ def current_trio_token(self): ki_pending = attr.ib(default=False) - # deliver_ki is broke. Maybe move all the actual logic and state into - # RunToken, and we'll only have one instance per runner? But then we can't - # have a public constructor. Eh, but current_run_token() returning a - # unique object per run feels pretty nice. Maybe let's just go for it. And - # keep the class public so people can isinstance() it if they want. - # This gets called from signal context def deliver_ki(self): self.ki_pending = True @@ -1512,9 +1493,14 @@ def deliver_ki(self): except RunFinishedError: pass + # The name of this function shows up in tracebacks, so make it a good one + async def _raise_deferred_keyboard_interrupt(self): + raise KeyboardInterrupt + def _deliver_ki_cb(self): if not self.ki_pending: return + # Can't happen because main_task and run_sync_soon_task are created at # the same time -- so even if KI arrives before main_task is created, # we won't get here until afterwards. @@ -1523,7 +1509,27 @@ def _deliver_ki_cb(self): # We're already in the process of exiting -- leave ki_pending set # and we'll check it again on our way out of run(). return - self.main_task._attempt_delivery_of_pending_ki() + + # Raise KI from a new task in the innermost nursery that was opened + # by the main task. Rationale: + # - Using a new task means we don't have to contend with + # injecting KI at a checkpoint in an existing task. + # - Either the main task has at least one nursery open, or there are + # no non-system tasks except the main task. + # - The main task is likely to be waiting in __aexit__ of its innermost + # nursery. On Trio <=0.15.0, a deferred KI would be raised at the + # main task's next checkpoint. So, spawning our raise-KI task in the + # main task's innermost nursery is the most backwards-compatible + # thing we can do. + for nursery in reversed(self.main_task.child_nurseries): + if not nursery._closed: + self.ki_pending = False + nursery.start_soon(self._raise_deferred_keyboard_interrupt) + return + + # If we get here, the main task has no non-closed child nurseries. + # Cancel the whole run; we'll raise KI on our way out of run(). + self.system_nursery.cancel_scope.cancel() ################ # Quiescing @@ -1936,10 +1942,6 @@ def run_impl(runner, async_fn, args): elif type(msg) is WaitTaskRescheduled: task._cancel_points += 1 task._abort_func = msg.abort_func - # KI is "outside" all cancel scopes, so check for it - # before checking for regular cancellation: - if runner.ki_pending and task is runner.main_task: - task._attempt_delivery_of_pending_ki() task._attempt_delivery_of_any_pending_cancel() elif type(msg) is PermanentlyDetachCoroutineObject: # Pretend the task just exited with the given outcome diff --git a/trio/_core/_traps.py b/trio/_core/_traps.py index f340481078..33b4249416 100644 --- a/trio/_core/_traps.py +++ b/trio/_core/_traps.py @@ -1,3 +1,5 @@ +# coding: utf-8 + # These are the only functions that ever yield back to the task runner. import types @@ -94,7 +96,7 @@ async def wait_task_rescheduled(abort_func): timeout expiring). When this happens, the ``abort_func`` is called. Its interface looks like:: - def abort_func(raise_cancel): + def abort_func(exc): ... return trio.lowlevel.Abort.SUCCEEDED # or FAILED @@ -108,40 +110,43 @@ def abort_func(raise_cancel): task can't be cancelled at this time, and still has to make sure that "someone" eventually calls :func:`reschedule`. - At that point there are again two possibilities. You can simply ignore - the cancellation altogether: wait for the operation to complete and - then reschedule and continue as normal. (For example, this is what - :func:`trio.to_thread.run_sync` does if cancellation is disabled.) - The other possibility is that the ``abort_func`` does succeed in - cancelling the operation, but for some reason isn't able to report that - right away. (Example: on Windows, it's possible to request that an - async ("overlapped") I/O operation be cancelled, but this request is - *also* asynchronous – you don't find out until later whether the - operation was actually cancelled or not.) To report a delayed - cancellation, then you should reschedule the task yourself, and call - the ``raise_cancel`` callback passed to ``abort_func`` to raise a - :exc:`~trio.Cancelled` (or possibly :exc:`KeyboardInterrupt`) exception - into this task. Either of the approaches sketched below can work:: + At that point there are again two possibilities. You can simply + ignore the cancellation altogether: wait for the operation to + complete and then reschedule and continue as normal. (For + example, this is what :func:`trio.to_thread.run_sync` does if + cancellation is disabled.) The other possibility is that the + ``abort_func`` does succeed in cancelling the operation, but + for some reason isn't able to report that right away. (Example: + on Windows, it's possible to request that an async + ("overlapped") I/O operation be cancelled, but this request is + *also* asynchronous – you don't find out until later whether + the operation was actually cancelled or not.) To report a + delayed cancellation, you should reschedule the task yourself, + and cause it to raise the exception ``exc`` that was passed to + ``abort_func``. (Currently ``exc`` will always be a + `~trio.Cancelled` exception, but we may use this mechanism to + raise other exceptions in the future; you should raise whatever + you're given.) Either of the approaches sketched below can + work:: # Option 1: - # Catch the exception from raise_cancel and inject it into the task. + # Directly reschedule the task with the provided exception. # (This is what Trio does automatically for you if you return # Abort.SUCCEEDED.) - trio.lowlevel.reschedule(task, outcome.capture(raise_cancel)) + trio.lowlevel.reschedule(task, outcome.Error(exc)) # Option 2: # wait to be woken by "someone", and then decide whether to raise # the error from inside the task. - outer_raise_cancel = None - def abort(inner_raise_cancel): - nonlocal outer_raise_cancel - outer_raise_cancel = inner_raise_cancel + outer_exc = None + def abort(inner_exc): + nonlocal outer_exc + outer_exc = inner_exc TRY_TO_CANCEL_OPERATION() return trio.lowlevel.Abort.FAILED await wait_task_rescheduled(abort) if OPERATION_WAS_SUCCESSFULLY_CANCELLED: - # raises the error - outer_raise_cancel() + raise outer_exc In any case it's guaranteed that we only call the ``abort_func`` at most once per call to :func:`wait_task_rescheduled`. @@ -228,8 +233,8 @@ async def temporarily_detach_coroutine_object(abort_func): detached task directly without going through :func:`reattach_detached_coroutine_object`, which would be bad.) Your ``abort_func`` should still arrange for whatever the coroutine - object is doing to be cancelled, and then reattach to Trio and call - the ``raise_cancel`` callback, if possible. + object is doing to be cancelled, and then reattach to Trio and raise + the exception it received, if possible. Returns or raises whatever value or exception the new coroutine runner uses to resume the coroutine. diff --git a/trio/_core/tests/test_ki.py b/trio/_core/tests/test_ki.py index a63407484a..d24a3fda16 100644 --- a/trio/_core/tests/test_ki.py +++ b/trio/_core/tests/test_ki.py @@ -386,9 +386,8 @@ async def main(): ki_self() task = _core.current_task() - def abort(raise_cancel): - result = outcome.capture(raise_cancel) - _core.reschedule(task, result) + def abort(exc): + _core.reschedule(task, outcome.Error(exc)) return _core.Abort.FAILED with pytest.raises(KeyboardInterrupt): diff --git a/trio/_core/tests/test_run.py b/trio/_core/tests/test_run.py index e705af5c22..48206153d1 100644 --- a/trio/_core/tests/test_run.py +++ b/trio/_core/tests/test_run.py @@ -1539,9 +1539,8 @@ async def test_slow_abort_basic(): task = _core.current_task() token = _core.current_trio_token() - def slow_abort(raise_cancel): - result = outcome.capture(raise_cancel) - token.run_sync_soon(_core.reschedule, task, result) + def slow_abort(exc): + token.run_sync_soon(_core.reschedule, task, outcome.Error(exc)) return _core.Abort.FAILED await _core.wait_task_rescheduled(slow_abort) @@ -1554,10 +1553,9 @@ async def slow_aborter(): task = _core.current_task() token = _core.current_trio_token() - def slow_abort(raise_cancel): + def slow_abort(exc): record.append("abort-called") - result = outcome.capture(raise_cancel) - token.run_sync_soon(_core.reschedule, task, result) + token.run_sync_soon(_core.reschedule, task, outcome.Error(exc)) return _core.Abort.FAILED with pytest.raises(_core.Cancelled): From ba74c8f2044f8b5ab7b6d552a6b30cfbf07aede4 Mon Sep 17 00:00:00 2001 From: Joshua Oreman Date: Wed, 20 May 2020 08:30:45 +0000 Subject: [PATCH 2/9] Add tests --- trio/_core/_exceptions.py | 2 +- trio/_core/_run.py | 5 +-- trio/_core/tests/test_ki.py | 81 +++++++++++++++++++++++++----------- trio/_core/tests/test_run.py | 17 ++++++++ 4 files changed, 76 insertions(+), 29 deletions(-) diff --git a/trio/_core/_exceptions.py b/trio/_core/_exceptions.py index 0887c3849a..ef70eb3df5 100644 --- a/trio/_core/_exceptions.py +++ b/trio/_core/_exceptions.py @@ -76,7 +76,7 @@ def __call__(self): _deprecate.warn_deprecated( "wait_task_rescheduled's abort_fn taking a callback argument", "0.16.0", - issue=1536, + issue=1537, instead="an exception argument", ) raise self diff --git a/trio/_core/_run.py b/trio/_core/_run.py index 02a3a22bff..b1e522bea0 100644 --- a/trio/_core/_run.py +++ b/trio/_core/_run.py @@ -2056,10 +2056,7 @@ async def checkpoint_if_cancelled(): """ task = current_task() - if ( - task._cancel_status.effectively_cancelled or - (task is task._runner.main_task and task._runner.ki_pending) - ): + if task._cancel_status.effectively_cancelled: await _core.checkpoint() assert False # pragma: no cover task._cancel_points += 1 diff --git a/trio/_core/tests/test_ki.py b/trio/_core/tests/test_ki.py index d24a3fda16..e6c34ceb98 100644 --- a/trio/_core/tests/test_ki.py +++ b/trio/_core/tests/test_ki.py @@ -338,10 +338,11 @@ async def main(): async def main(): assert _core.currently_ki_protected() ki_self() - with pytest.raises(KeyboardInterrupt): + with pytest.raises(_core.Cancelled): await _core.checkpoint_if_cancelled() - _core.run(main) + with pytest.raises(KeyboardInterrupt): + _core.run(main) # KI arrives while main task is not abortable, b/c already scheduled print("check 6") @@ -353,10 +354,11 @@ async def main(): await _core.cancel_shielded_checkpoint() await _core.cancel_shielded_checkpoint() await _core.cancel_shielded_checkpoint() - with pytest.raises(KeyboardInterrupt): + with pytest.raises(_core.Cancelled): await _core.checkpoint() - _core.run(main) + with pytest.raises(KeyboardInterrupt): + _core.run(main) # KI arrives while main task is not abortable, b/c refuses to be aborted print("check 7") @@ -372,10 +374,11 @@ def abort(_): return _core.Abort.FAILED assert await _core.wait_task_rescheduled(abort) == 1 - with pytest.raises(KeyboardInterrupt): + with pytest.raises(_core.Cancelled): await _core.checkpoint() - _core.run(main) + with pytest.raises(KeyboardInterrupt): + _core.run(main) # KI delivered via slow abort print("check 8") @@ -390,11 +393,12 @@ def abort(exc): _core.reschedule(task, outcome.Error(exc)) return _core.Abort.FAILED - with pytest.raises(KeyboardInterrupt): + with pytest.raises(_core.Cancelled): assert await _core.wait_task_rescheduled(abort) await _core.checkpoint() - _core.run(main) + with pytest.raises(KeyboardInterrupt): + _core.run(main) # KI arrives just before main task exits, so the run_sync_soon machinery # is still functioning and will accept the callback to deliver the KI, but @@ -421,10 +425,11 @@ async def main(): # ...but even after the KI, we keep running uninterrupted... record.append("ok") # ...until we hit a checkpoint: - with pytest.raises(KeyboardInterrupt): + with pytest.raises(_core.Cancelled): await sleep(10) - _core.run(main, restrict_keyboard_interrupt_to_checkpoints=True) + with pytest.raises(KeyboardInterrupt): + _core.run(main, restrict_keyboard_interrupt_to_checkpoints=True) assert record == ["ok"] record = [] # Exact same code raises KI early if we leave off the argument, doesn't @@ -433,24 +438,51 @@ async def main(): _core.run(main) assert record == [] - # KI arrives while main task is inside a cancelled cancellation scope - # the KeyboardInterrupt should take priority print("check 11") - + # KI delivered into innermost main task nursery if there are any @_core.enable_ki_protection async def main(): - assert _core.currently_ki_protected() - with _core.CancelScope() as cancel_scope: - cancel_scope.cancel() - with pytest.raises(_core.Cancelled): - await _core.checkpoint() - ki_self() + async with _core.open_nursery() as outer: with pytest.raises(KeyboardInterrupt): - await _core.checkpoint() - with pytest.raises(_core.Cancelled): - await _core.checkpoint() + async with _core.open_nursery() as inner: + ki_self() + record.append("ok") + # First tick ensures KI callback ran + # Second tick ensures KI delivery task ran + await _core.cancel_shielded_checkpoint() + await _core.cancel_shielded_checkpoint() + with pytest.raises(_core.Cancelled): + await _core.checkpoint() + record.append("ok 2") + record.append("ok 3") + record.append("ok 4") _core.run(main) + assert record == ["ok", "ok 2", "ok 3", "ok 4"] + + # Closed nurseries are ignored when picking one to deliver KI + print("check 12") + record = [] + + @_core.enable_ki_protection + async def main(): + with pytest.raises(KeyboardInterrupt): + async with _core.open_nursery() as outer: + async with _core.open_nursery() as inner: + assert inner._closed is False + inner._closed = True + ki_self() + # First tick ensures KI callback ran + # Second tick ensures KI delivery task ran + await _core.cancel_shielded_checkpoint() + await _core.cancel_shielded_checkpoint() + record.append("ok") + record.append("nope") # pragma: no cover + record.append("ok 2") + + _core.run(main) + assert record == ["ok", "ok 2"] + def test_ki_is_good_neighbor(): @@ -574,7 +606,7 @@ async def main(): print("Starting thread") thread.start() try: - with pytest.raises(KeyboardInterrupt): + with pytest.raises(_core.Cancelled): # To limit the damage on CI if this does get broken (as # compared to sleep_forever()) print("Going to sleep") @@ -606,7 +638,8 @@ async def main(): start = time.perf_counter() try: - _core.run(main) + with pytest.raises(KeyboardInterrupt): + _core.run(main) finally: end = time.perf_counter() print("duration", end - start) diff --git a/trio/_core/tests/test_run.py b/trio/_core/tests/test_run.py index 48206153d1..3e6b9158b6 100644 --- a/trio/_core/tests/test_run.py +++ b/trio/_core/tests/test_run.py @@ -17,6 +17,7 @@ from .tutil import slow, check_sequence_matches, gc_collect_harder from ... import _core +from ..._deprecate import TrioDeprecationWarning from ..._threads import to_thread_run_sync from ..._timeouts import sleep, fail_after from ...testing import ( @@ -1532,6 +1533,22 @@ def cb(i): assert counter[0] == COUNT +async def test_deprecated_abort_fn_semantics(): + with _core.CancelScope() as scope: + scope.cancel() + with pytest.raises(_core.Cancelled): + task = _core.current_task() + token = _core.current_trio_token() + + def slow_abort(raise_cancel): + with pytest.warns(TrioDeprecationWarning): + result = outcome.capture(raise_cancel) + token.run_sync_soon(_core.reschedule, task, result) + return _core.Abort.FAILED + + await _core.wait_task_rescheduled(slow_abort) + + async def test_slow_abort_basic(): with _core.CancelScope() as scope: scope.cancel() From 2b780a71b24383f98fb949d08f39a7543d33e2c4 Mon Sep 17 00:00:00 2001 From: Joshua Oreman Date: Wed, 20 May 2020 08:35:37 +0000 Subject: [PATCH 3/9] yapf --- trio/_core/tests/test_ki.py | 1 - 1 file changed, 1 deletion(-) diff --git a/trio/_core/tests/test_ki.py b/trio/_core/tests/test_ki.py index e6c34ceb98..4221331afd 100644 --- a/trio/_core/tests/test_ki.py +++ b/trio/_core/tests/test_ki.py @@ -484,7 +484,6 @@ async def main(): assert record == ["ok", "ok 2"] - def test_ki_is_good_neighbor(): # in the unlikely event someone overwrites our signal handler, we leave # the overwritten one be From eaea7854b1c3a3be1c745cb225530efa77a2d545 Mon Sep 17 00:00:00 2001 From: Joshua Oreman Date: Wed, 20 May 2020 08:40:40 +0000 Subject: [PATCH 4/9] flake8 --- trio/_core/tests/test_ki.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/trio/_core/tests/test_ki.py b/trio/_core/tests/test_ki.py index 4221331afd..825284b21d 100644 --- a/trio/_core/tests/test_ki.py +++ b/trio/_core/tests/test_ki.py @@ -442,9 +442,9 @@ async def main(): # KI delivered into innermost main task nursery if there are any @_core.enable_ki_protection async def main(): - async with _core.open_nursery() as outer: + async with _core.open_nursery(): with pytest.raises(KeyboardInterrupt): - async with _core.open_nursery() as inner: + async with _core.open_nursery(): ki_self() record.append("ok") # First tick ensures KI callback ran @@ -467,7 +467,7 @@ async def main(): @_core.enable_ki_protection async def main(): with pytest.raises(KeyboardInterrupt): - async with _core.open_nursery() as outer: + async with _core.open_nursery(): async with _core.open_nursery() as inner: assert inner._closed is False inner._closed = True From 8a089d05b838d2a4ed96d5ed365a987ff963faed Mon Sep 17 00:00:00 2001 From: Joshua Oreman Date: Wed, 20 May 2020 09:05:09 +0000 Subject: [PATCH 5/9] Resolve drop in coverage --- trio/_core/tests/test_ki.py | 1 + 1 file changed, 1 insertion(+) diff --git a/trio/_core/tests/test_ki.py b/trio/_core/tests/test_ki.py index 825284b21d..fcab24135a 100644 --- a/trio/_core/tests/test_ki.py +++ b/trio/_core/tests/test_ki.py @@ -445,6 +445,7 @@ async def main(): async with _core.open_nursery(): with pytest.raises(KeyboardInterrupt): async with _core.open_nursery(): + ki_self() ki_self() record.append("ok") # First tick ensures KI callback ran From b18396c58d3b287104a80b7124e4cb10e15a20a3 Mon Sep 17 00:00:00 2001 From: Joshua Oreman Date: Fri, 22 May 2020 07:35:35 +0000 Subject: [PATCH 6/9] add notes about delivery --- docs/source/reference-lowlevel.rst | 27 ++++++++++++++++++++++----- 1 file changed, 22 insertions(+), 5 deletions(-) diff --git a/docs/source/reference-lowlevel.rst b/docs/source/reference-lowlevel.rst index 53e5cf8a8c..3e6cc8a4e2 100644 --- a/docs/source/reference-lowlevel.rst +++ b/docs/source/reference-lowlevel.rst @@ -226,11 +226,11 @@ Windows-specific API .. function:: WaitForSingleObject(handle) :async: - + Async and cancellable variant of `WaitForSingleObject `__. Windows only. - + :arg handle: A Win32 object handle, as a Python integer. :raises OSError: @@ -282,9 +282,26 @@ an infinite loop, we do want to be able to break out of that. Our solution is to install a default signal handler which checks whether it's safe to raise :exc:`KeyboardInterrupt` at the place where the signal is received. If so, then we do; otherwise, we schedule a -:exc:`KeyboardInterrupt` to be delivered to the main task at the next -available opportunity (similar to how :exc:`~trio.Cancelled` is -delivered). +:exc:`KeyboardInterrupt` to be delivered sometime soon. + +.. note:: Delivery "sometime soon" is accomplished by picking an open + nursery and spawning a new task there that raises + `KeyboardInterrupt`. Like any other unhandled exception, this will + cancel sibling tasks as it propagates, and ultimately escape from + the call to :func:`trio.run` unless caught sooner. + + It's not a good idea to try to catch `KeyboardInterrupt` while + you're still inside Trio, because it might be raised anywhere, + including outside your ``try``/``except`` block. If you want Ctrl+C + to do something that's not "tear down all running tasks", then you + should use :func:`open_signal_receiver` to install a handler for + ``SIGINT``. If you do that, then Ctrl+C will go to your handler rather + than using the default handling described in this section. + + The details of which nursery gets the `KeyboardInterrupt` injected + are subject to change. Currently it's the innermost nursery + that's active in the main task (the one running the original function + you passed to :func:`trio.run`). So that's great, but – how do we know whether we're in one of the sensitive parts of the program or not? From aee96618750aff55390d9547a1642ab54f1383b0 Mon Sep 17 00:00:00 2001 From: Joshua Oreman Date: Fri, 22 May 2020 09:16:10 +0000 Subject: [PATCH 7/9] Always inject a deferred KI at the top level of trio.run(), per discussion in #151 --- docs/source/reference-lowlevel.rst | 36 ++++++++++------------- trio/_core/_run.py | 42 ++------------------------- trio/_core/tests/test_ki.py | 46 ------------------------------ 3 files changed, 18 insertions(+), 106 deletions(-) diff --git a/docs/source/reference-lowlevel.rst b/docs/source/reference-lowlevel.rst index 3e6cc8a4e2..992ff88087 100644 --- a/docs/source/reference-lowlevel.rst +++ b/docs/source/reference-lowlevel.rst @@ -281,27 +281,21 @@ correctness invariants. On the other, if the user accidentally writes an infinite loop, we do want to be able to break out of that. Our solution is to install a default signal handler which checks whether it's safe to raise :exc:`KeyboardInterrupt` at the place where the -signal is received. If so, then we do; otherwise, we schedule a -:exc:`KeyboardInterrupt` to be delivered sometime soon. - -.. note:: Delivery "sometime soon" is accomplished by picking an open - nursery and spawning a new task there that raises - `KeyboardInterrupt`. Like any other unhandled exception, this will - cancel sibling tasks as it propagates, and ultimately escape from - the call to :func:`trio.run` unless caught sooner. - - It's not a good idea to try to catch `KeyboardInterrupt` while - you're still inside Trio, because it might be raised anywhere, - including outside your ``try``/``except`` block. If you want Ctrl+C - to do something that's not "tear down all running tasks", then you - should use :func:`open_signal_receiver` to install a handler for - ``SIGINT``. If you do that, then Ctrl+C will go to your handler rather - than using the default handling described in this section. - - The details of which nursery gets the `KeyboardInterrupt` injected - are subject to change. Currently it's the innermost nursery - that's active in the main task (the one running the original function - you passed to :func:`trio.run`). +signal is received. If so, then we do. Otherwise, we cancel all tasks +and raise `KeyboardInterrupt` directly as the result of :func:`trio.run`. + +.. note:: This behavior means it's not a good idea to try to catch + `KeyboardInterrupt` within a Trio task. Most Trio + programs are I/O-bound, so most interrupts will be received while + no task is running (because Trio is waiting for I/O). There's no + task that should obviously receive the interrupt in such cases, so + Trio doesn't raise it within a task at all: every task gets cancelled, + then `KeyboardInterrupt` is raised once that's complete. + + If you want to handle Ctrl+C by doing something other than "cancel + all tasks", then you should use :func:`open_signal_receiver` to + install a handler for ``SIGINT``. If you do that, then Ctrl+C will + go to your handler, and it can do whatever it wants. So that's great, but – how do we know whether we're in one of the sensitive parts of the program or not? diff --git a/trio/_core/_run.py b/trio/_core/_run.py index 64cc6aa347..41573d9280 100644 --- a/trio/_core/_run.py +++ b/trio/_core/_run.py @@ -1412,48 +1412,12 @@ def current_trio_token(self): def deliver_ki(self): self.ki_pending = True try: - self.entry_queue.run_sync_soon(self._deliver_ki_cb) + self.entry_queue.run_sync_soon( + self.system_nursery.cancel_scope.cancel + ) except RunFinishedError: pass - # The name of this function shows up in tracebacks, so make it a good one - async def _raise_deferred_keyboard_interrupt(self): - raise KeyboardInterrupt - - def _deliver_ki_cb(self): - if not self.ki_pending: - return - - # Can't happen because main_task and run_sync_soon_task are created at - # the same time -- so even if KI arrives before main_task is created, - # we won't get here until afterwards. - assert self.main_task is not None - if self.main_task_outcome is not None: - # We're already in the process of exiting -- leave ki_pending set - # and we'll check it again on our way out of run(). - return - - # Raise KI from a new task in the innermost nursery that was opened - # by the main task. Rationale: - # - Using a new task means we don't have to contend with - # injecting KI at a checkpoint in an existing task. - # - Either the main task has at least one nursery open, or there are - # no non-system tasks except the main task. - # - The main task is likely to be waiting in __aexit__ of its innermost - # nursery. On Trio <=0.15.0, a deferred KI would be raised at the - # main task's next checkpoint. So, spawning our raise-KI task in the - # main task's innermost nursery is the most backwards-compatible - # thing we can do. - for nursery in reversed(self.main_task.child_nurseries): - if not nursery._closed: - self.ki_pending = False - nursery.start_soon(self._raise_deferred_keyboard_interrupt) - return - - # If we get here, the main task has no non-closed child nurseries. - # Cancel the whole run; we'll raise KI on our way out of run(). - self.system_nursery.cancel_scope.cancel() - ################ # Quiescing ################ diff --git a/trio/_core/tests/test_ki.py b/trio/_core/tests/test_ki.py index fcab24135a..ddbf10c8e5 100644 --- a/trio/_core/tests/test_ki.py +++ b/trio/_core/tests/test_ki.py @@ -438,52 +438,6 @@ async def main(): _core.run(main) assert record == [] - print("check 11") - # KI delivered into innermost main task nursery if there are any - @_core.enable_ki_protection - async def main(): - async with _core.open_nursery(): - with pytest.raises(KeyboardInterrupt): - async with _core.open_nursery(): - ki_self() - ki_self() - record.append("ok") - # First tick ensures KI callback ran - # Second tick ensures KI delivery task ran - await _core.cancel_shielded_checkpoint() - await _core.cancel_shielded_checkpoint() - with pytest.raises(_core.Cancelled): - await _core.checkpoint() - record.append("ok 2") - record.append("ok 3") - record.append("ok 4") - - _core.run(main) - assert record == ["ok", "ok 2", "ok 3", "ok 4"] - - # Closed nurseries are ignored when picking one to deliver KI - print("check 12") - record = [] - - @_core.enable_ki_protection - async def main(): - with pytest.raises(KeyboardInterrupt): - async with _core.open_nursery(): - async with _core.open_nursery() as inner: - assert inner._closed is False - inner._closed = True - ki_self() - # First tick ensures KI callback ran - # Second tick ensures KI delivery task ran - await _core.cancel_shielded_checkpoint() - await _core.cancel_shielded_checkpoint() - record.append("ok") - record.append("nope") # pragma: no cover - record.append("ok 2") - - _core.run(main) - assert record == ["ok", "ok 2"] - def test_ki_is_good_neighbor(): # in the unlikely event someone overwrites our signal handler, we leave From ed3fe73d49213ec94e8cc3e8c9866fc714660eda Mon Sep 17 00:00:00 2001 From: Joshua Oreman Date: Fri, 22 May 2020 09:29:59 +0000 Subject: [PATCH 8/9] Fix docs --- docs/source/reference-lowlevel.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/source/reference-lowlevel.rst b/docs/source/reference-lowlevel.rst index 992ff88087..6a79cadc64 100644 --- a/docs/source/reference-lowlevel.rst +++ b/docs/source/reference-lowlevel.rst @@ -293,7 +293,7 @@ and raise `KeyboardInterrupt` directly as the result of :func:`trio.run`. then `KeyboardInterrupt` is raised once that's complete. If you want to handle Ctrl+C by doing something other than "cancel - all tasks", then you should use :func:`open_signal_receiver` to + all tasks", then you should use :func:`~trio.open_signal_receiver` to install a handler for ``SIGINT``. If you do that, then Ctrl+C will go to your handler, and it can do whatever it wants. From 502842664b9d6c6425adb44270166760da67a8c4 Mon Sep 17 00:00:00 2001 From: Joshua Oreman Date: Fri, 22 May 2020 09:55:17 +0000 Subject: [PATCH 9/9] Add newsfragments --- docs/source/reference-lowlevel.rst | 2 ++ newsfragments/1537.breaking.rst | 42 ++++++++++++++++++++++++++++++ newsfragments/1537.removal.rst | 4 +++ pyproject.toml | 5 ++++ 4 files changed, 53 insertions(+) create mode 100644 newsfragments/1537.breaking.rst create mode 100644 newsfragments/1537.removal.rst diff --git a/docs/source/reference-lowlevel.rst b/docs/source/reference-lowlevel.rst index 6a79cadc64..b1ab14f822 100644 --- a/docs/source/reference-lowlevel.rst +++ b/docs/source/reference-lowlevel.rst @@ -270,6 +270,8 @@ Trio tokens .. autofunction:: current_trio_token +.. _ki-handling: + Safer KeyboardInterrupt handling ================================ diff --git a/newsfragments/1537.breaking.rst b/newsfragments/1537.breaking.rst new file mode 100644 index 0000000000..cf87925ce4 --- /dev/null +++ b/newsfragments/1537.breaking.rst @@ -0,0 +1,42 @@ +:ref:`Sometimes `, a Trio program receives an interrupt +signal (Ctrl+C) at a time when Python's default response (raising +`KeyboardInterrupt` immediately) might corrupt Trio's internal +state. Previously, Trio would handle this situation by raising the +`KeyboardInterrupt` at the next :ref:`checkpoint ` executed +by the main task (the one running the function you passed to :func:`trio.run`). +This was responsible for a lot of internal complexity and sometimes led to +surprising behavior. + +With this release, such a "deferred" `KeyboardInterrupt` is handled in a +different way: Trio will first cancel all running tasks, then raise +`KeyboardInterrupt` directly out of the call to :func:`trio.run`. +The difference is relevant if you have code that tries to catch +`KeyboardInterrupt` within Trio. This was never entirely robust, but it +previously might have worked in many cases, whereas now it will never +catch the interrupt. + +An example of code that mostly worked on previous releases, but won't +work on this release:: + + async def main(): + try: + await trio.sleep_forever() + except KeyboardInterrupt: + print("interrupted") + trio.run(main) + +The fix is to catch `KeyboardInterrupt` outside Trio:: + + async def main(): + await trio.sleep_forever() + try: + trio.run(main) + except KeyboardInterrupt: + print("interrupted") + +If that doesn't work for you (because you want to respond to +`KeyboardInterrupt` by doing something other than cancelling all +tasks), then you can start a task that uses +`trio.open_signal_receiver` to receive the interrupt signal ``SIGINT`` +directly and handle it however you wish. Such a task takes precedence +over Trio's default interrupt handling. diff --git a/newsfragments/1537.removal.rst b/newsfragments/1537.removal.rst new file mode 100644 index 0000000000..0c4aee38a7 --- /dev/null +++ b/newsfragments/1537.removal.rst @@ -0,0 +1,4 @@ +The abort function passed to :func:`~trio.lowlevel.wait_task_rescheduled` +now directly takes as argument the cancellation exception that should be +raised after a successful asynchronous cancellation. Previously, it took +a callable that would raise the exception when called. diff --git a/pyproject.toml b/pyproject.toml index 768a4766eb..6100edcba8 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -14,6 +14,11 @@ issue_format = "`#{issue} `_ # Unfortunately there's no way to simply override # tool.towncrier.type.misc.showcontent +[[tool.towncrier.type]] +directory = "breaking" +name = "Breaking Changes" +showcontent = true + [[tool.towncrier.type]] directory = "feature" name = "Features"