From 23c089d5abffe95d95f51db774ba6ca600d465ec Mon Sep 17 00:00:00 2001 From: Joshua Oreman Date: Wed, 7 Aug 2019 15:48:49 +0100 Subject: [PATCH 1/5] Recover from a 'foreign await' by throwing in the TypeError This improves debuggability (since the traceback points to the location of the error, rather than just the nursery whose task was at fault) and fixes #552 by ensuring we don't abandon the child tasks of the errant one. --- newsfragments/552.bugfix.rst | 1 + trio/_core/_run.py | 34 ++++++++++++++++++---------------- trio/_core/tests/test_run.py | 7 ++++++- 3 files changed, 25 insertions(+), 17 deletions(-) create mode 100644 newsfragments/552.bugfix.rst diff --git a/newsfragments/552.bugfix.rst b/newsfragments/552.bugfix.rst new file mode 100644 index 0000000000..8239edcf25 --- /dev/null +++ b/newsfragments/552.bugfix.rst @@ -0,0 +1 @@ +When a Trio task makes improper use of a non-Trio async library, Trio now causes an exception to be raised within the task at the point of the error, rather than abandoning the task and raising an error in its parent. This improves debuggability and resolves the `TrioInternalError` that would sometimes result from the latter strategy. diff --git a/trio/_core/_run.py b/trio/_core/_run.py index 96da9262bf..330a250772 100644 --- a/trio/_core/_run.py +++ b/trio/_core/_run.py @@ -980,10 +980,14 @@ class Task: _counter = attr.ib(init=False, factory=itertools.count().__next__) # Invariant: - # - for unscheduled tasks, _next_send is None - # - for scheduled tasks, _next_send is an Outcome object, - # and custom_sleep_data is None + # - for unscheduled tasks, _next_send_fn and _next_send are both None + # - for scheduled tasks, _next_send_fn(_next_send) resumes the task; + # usually _next_send_fn is self.coro.send and _next_send is an + # Outcome. When recovering from a foreign await, _next_send_fn is + # self.coro.throw and _next_send is an exception. + # - for scheduled tasks, custom_sleep_data is None # Tasks start out unscheduled. + _next_send_fn = attr.ib(default=None) _next_send = attr.ib(default=None) _abort_func = attr.ib(default=None) custom_sleep_data = attr.ib(default=None) @@ -1215,7 +1219,8 @@ def reschedule(self, task, next_send=_NO_SEND): next_send = Value(None) assert task._runner is self - assert task._next_send is None + assert task._next_send_fn is None + task._next_send_fn = task.coro.send task._next_send = next_send task._abort_func = None task.custom_sleep_data = None @@ -1887,8 +1892,9 @@ def run_impl(runner, async_fn, args): if runner.instruments: runner.instrument("before_task_step", task) + next_send_fn = task._next_send_fn next_send = task._next_send - task._next_send = None + task._next_send_fn = task._next_send = None final_outcome = None try: # We used to unwrap the Outcome object here and send/throw its @@ -1898,7 +1904,7 @@ def run_impl(runner, async_fn, args): # https://bugs.python.org/issue29590 # So now we send in the Outcome object and unwrap it on the # other side. - msg = task.context.run(task.coro.send, next_send) + msg = task.context.run(next_send_fn, next_send) except StopIteration as stop_iteration: final_outcome = Value(stop_iteration.value) except BaseException as task_exc: @@ -1938,16 +1944,12 @@ def run_impl(runner, async_fn, args): "other framework like asyncio? That won't work " "without some kind of compatibility shim.".format(msg) ) - # How can we resume this task? It's blocked in code we - # don't control, waiting for some message that we know - # nothing about. We *could* try using coro.throw(...) to - # blast an exception in and hope that it propagates out, - # but (a) that's complicated because we aren't set up to - # resume a task via .throw(), and (b) even if we did, - # there's no guarantee that the foreign code will respond - # the way we're hoping. So instead we abandon this task - # and propagate the exception into the task's spawner. - runner.task_exited(task, Error(exc)) + # The foreign library probably doesn't adhere to our + # protocol of unwrapping whatever outcome gets sent in. + # Instead, we'll arrange to throw `exc` in directly, + # which works for at least asyncio and curio. + runner.reschedule(task, exc) + task._next_send_fn = task.coro.throw if runner.instruments: runner.instrument("after_task_step", task) diff --git a/trio/_core/tests/test_run.py b/trio/_core/tests/test_run.py index 07798be935..52ca1aab06 100644 --- a/trio/_core/tests/test_run.py +++ b/trio/_core/tests/test_run.py @@ -1782,14 +1782,19 @@ async def async_gen(arg): # pragma: no cover def test_calling_asyncio_function_gives_nice_error(): - async def misguided(): + async def child_xyzzy(): import asyncio await asyncio.Future() + async def misguided(): + await child_xyzzy() + with pytest.raises(TypeError) as excinfo: _core.run(misguided) assert "asyncio" in str(excinfo.value) + # The traceback should point to the location of the foreign await + assert any(entry.name == "child_xyzzy" for entry in excinfo.traceback) async def test_trivial_yields(): From 0aada63b927d458b20f1da69a4818651e33c1ec2 Mon Sep 17 00:00:00 2001 From: Joshua Oreman Date: Thu, 8 Aug 2019 01:51:12 +0200 Subject: [PATCH 2/5] Add regression test for #552 --- trio/_core/tests/test_run.py | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/trio/_core/tests/test_run.py b/trio/_core/tests/test_run.py index 52ca1aab06..a5345424ed 100644 --- a/trio/_core/tests/test_run.py +++ b/trio/_core/tests/test_run.py @@ -1797,6 +1797,16 @@ async def misguided(): assert any(entry.name == "child_xyzzy" for entry in excinfo.traceback) +async def test_asyncio_function_inside_nursery_does_not_explode(): + # Regression test for https://github.com/python-trio/trio/issues/552 + with pytest.raises(TypeError) as excinfo: + async with _core.open_nursery() as nursery: + import asyncio + nursery.start_soon(sleep_forever) + await asyncio.Future() + assert "asyncio" in str(excinfo.value) + + async def test_trivial_yields(): with assert_checkpoints(): await _core.checkpoint() From 0a96de005a85747a82310eadc6be483c137013f6 Mon Sep 17 00:00:00 2001 From: Joshua Oreman Date: Thu, 8 Aug 2019 01:58:10 +0200 Subject: [PATCH 3/5] Explain the traceback thing --- trio/_core/_run.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/trio/_core/_run.py b/trio/_core/_run.py index 330a250772..d662d0eb52 100644 --- a/trio/_core/_run.py +++ b/trio/_core/_run.py @@ -984,7 +984,10 @@ class Task: # - for scheduled tasks, _next_send_fn(_next_send) resumes the task; # usually _next_send_fn is self.coro.send and _next_send is an # Outcome. When recovering from a foreign await, _next_send_fn is - # self.coro.throw and _next_send is an exception. + # self.coro.throw and _next_send is an exception. _next_send_fn + # will effectively be at the top of every task's call stack, so + # it should be written in C if you don't want to pollute Trio + # tracebacks with extraneous frames. # - for scheduled tasks, custom_sleep_data is None # Tasks start out unscheduled. _next_send_fn = attr.ib(default=None) From 429d75f9fc59f2beb4800b3a6b129d672436bb2e Mon Sep 17 00:00:00 2001 From: "Nathaniel J. Smith" Date: Sat, 10 Aug 2019 19:57:11 -0700 Subject: [PATCH 4/5] Add pragma: no branch This early exit is totally uninteresting, no reason for coverage to hassle us over it. --- trio/_core/tests/test_run.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/trio/_core/tests/test_run.py b/trio/_core/tests/test_run.py index a5345424ed..d3e33f155e 100644 --- a/trio/_core/tests/test_run.py +++ b/trio/_core/tests/test_run.py @@ -1794,7 +1794,7 @@ async def misguided(): assert "asyncio" in str(excinfo.value) # The traceback should point to the location of the foreign await - assert any(entry.name == "child_xyzzy" for entry in excinfo.traceback) + assert any(entry.name == "child_xyzzy" for entry in excinfo.traceback) # pragma: no branch async def test_asyncio_function_inside_nursery_does_not_explode(): From 3f32860fcbc9f0b4217f271da187a07af899a9c2 Mon Sep 17 00:00:00 2001 From: "Nathaniel J. Smith" Date: Mon, 12 Aug 2019 19:05:21 -0700 Subject: [PATCH 5/5] Try to make yapf and coverage happy at the same time --- trio/_core/tests/test_run.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/trio/_core/tests/test_run.py b/trio/_core/tests/test_run.py index d3e33f155e..f1ca40f4d4 100644 --- a/trio/_core/tests/test_run.py +++ b/trio/_core/tests/test_run.py @@ -1794,7 +1794,9 @@ async def misguided(): assert "asyncio" in str(excinfo.value) # The traceback should point to the location of the foreign await - assert any(entry.name == "child_xyzzy" for entry in excinfo.traceback) # pragma: no branch + assert any( # pragma: no branch + entry.name == "child_xyzzy" for entry in excinfo.traceback + ) async def test_asyncio_function_inside_nursery_does_not_explode():