Merge pull request #83 from njsmith/fixture-yield-is-checkpoint

Rework handling of cancelled 'yield' in fixtures

Author: belm0

docs/source/reference.rst

@@ -106,6 +106,114 @@ but Trio fixtures **must be test scoped**. Class, module, and session
 scope are not supported.
 
 
+.. _cancel-yield:
+
+An important note about ``yield`` fixtures
+------------------------------------------
+
+Like any pytest fixture, Trio fixtures can contain both setup and
+teardown code separated by a ``yield``::
+
+   @pytest.fixture
+   async def my_fixture():
+       ... setup code ...
+       yield
+       ... teardown code ...
+
+When pytest-trio executes this fixture, it creates a new task, and
+runs the setup code until it reaches the ``yield``. Then the fixture's
+task goes to sleep. Once the test has finished, the fixture task wakes
+up again and resumes at the ``yield``, so it can execute the teardown
+code.
+
+So the ``yield`` in a fixture is sort of like calling ``await
+wait_for_test_to_finish()``. And in Trio, any ``await``\-able
+operation can be cancelled. For example, we could put a timeout on the
+``yield``::
+
+   @pytest.fixture
+   async def my_fixture():
+       ... setup code ...
+       with trio.move_on_after(5):
+           yield  # this yield gets cancelled after 5 seconds
+       ... teardown code ...
+
+Now if the test takes more than 5 seconds to execute, this fixture
+will cancel the ``yield``.
+
+That's kind of a strange thing to do, but there's another version of
+this that's extremely common. Suppose your fixture spawns a background
+task, and then the background task raises an exception. Whenever a
+background task raises an exception, it automatically cancels
+everything inside the nursery's scope – which includes our ``yield``::
+
+   @pytest.fixture
+   async def my_fixture(nursery):
+       nursery.start_soon(function_that_raises_exception)
+       yield   # this yield gets cancelled after the background task crashes
+       ... teardown code ...
+
+If you use fixtures with background tasks, you'll probably end up
+cancelling one of these ``yield``\s sooner or later. So what happens
+if the ``yield`` gets cancelled?
+
+First, pytest-trio assumes that something has gone wrong and there's
+no point in continuing the test. If the top-level test function is
+running, then it cancels it.
+
+Then, pytest-trio waits for the test function to finish, and
+then begins tearing down fixtures as normal.
+
+During this teardown process, it will eventually reach the fixture
+that cancelled its ``yield``. This fixture gets resumed to execute its
+teardown logic, but with a special twist: since the ``yield`` was
+cancelled, the ``yield`` raises :exc:`trio.Cancelled`.
+
+Now, here's the punchline: this means that in our examples above, the
+teardown code might not be executed at all! **This is different from
+how pytest fixtures normally work.** Normally, the ``yield`` in a
+pytest fixture never raises an exception, so you can be certain that
+any code you put after it will execute as normal. But if you have a
+fixture with background tasks, and they crash, then your ``yield``
+might raise an exception, and Python will skip executing the code
+after the ``yield``.
+
+In our experience, most fixtures are fine with this, and it prevents
+some `weird problems
+<https://github.com/python-trio/pytest-trio/issues/75>`__ that can
+happen otherwise. But it's something to be aware of.
+
+If you have a fixture where the ``yield`` might be cancelled but you
+still need to run teardown code, then you can use a ``finally``
+block::
+
+   @pytest.fixture
+   async def my_fixture(nursery):
+       nursery.start_soon(function_that_crashes)
+       try:
+           # This yield could be cancelled...
+           yield
+       finally:
+           # But this code will run anyway
+           ... teardown code ...
+
+(But, watch out: the teardown code is still running in a cancelled
+context, so if it has any ``await``\s it could raise
+:exc:`trio.Cancelled` again.)
+
+Or if you use ``with`` to handle teardown, then you don't have to
+worry about this because ``with`` blocks always perform cleanup even
+if there's an exception::
+
+   @pytest.fixture
+   async def my_fixture(nursery):
+       with get_obj_that_must_be_torn_down() as obj:
+           nursery.start_soon(function_that_crashes, obj)
+           # This could raise trio.Cancelled...
+           # ...but that's OK, the 'with' block will still tear down 'obj'
+           yield obj
+
+
 Concurrent setup/teardown
 -------------------------
 

newsfragments/75.feature.rst

@@ -0,0 +1,6 @@
+Incompatible change: if you use ``yield`` inside a Trio fixture, and
+the ``yield`` gets cancelled (for example, due to a background task
+crashing), then the ``yield`` will now raise :exc:`trio.Cancelled`.
+See :ref:`cancel-yield` for details. Also, in this same case,
+pytest-trio will now reliably mark the test as failed, even if the
+fixture doesn't go on to raise an exception.

pytest_trio/_tests/test_fixture_mistakes.py

@@ -117,3 +117,32 @@ def test_whatever(async_fixture):
     result.stdout.fnmatch_lines(
         ["*: Trio fixtures can only be used by Trio tests*"]
     )
+
+
+@enable_trio_mode
+def test_fixture_cancels_test_but_doesnt_raise(testdir, enable_trio_mode):
+    enable_trio_mode(testdir)
+
+    testdir.makepyfile(
+        """
+        import pytest
+        import trio
+        from async_generator import async_generator, yield_
+
+        @pytest.fixture
+        @async_generator
+        async def async_fixture():
+            with trio.CancelScope() as cscope:
+                cscope.cancel()
+                await yield_()
+
+
+        async def test_whatever(async_fixture):
+            pass
+        """
+    )
+
+    result = testdir.runpytest()
+
+    result.assert_outcomes(failed=1)
+    result.stdout.fnmatch_lines(["*async_fixture*cancelled the test*"])

pytest_trio/_tests/test_fixture_ordering.py

@@ -213,7 +213,8 @@ def test_background_crash_cancellation_propagation(bgmode, testdir):
         @trio_fixture
         def crashyfix(nursery):
             nursery.start_soon(crashy)
-            yield
+            with pytest.raises(trio.Cancelled):
+                yield
             # We should be cancelled here
             teardown_deadlines["crashyfix"] = trio.current_effective_deadline()
         """
@@ -224,7 +225,8 @@ def crashyfix(nursery):
         async def crashyfix():
             async with trio.open_nursery() as nursery:
                 nursery.start_soon(crashy)
-                await yield_()
+                with pytest.raises(trio.Cancelled):
+                    await yield_()
                 # We should be cancelled here
                 teardown_deadlines["crashyfix"] = trio.current_effective_deadline()
         """
@@ -284,3 +286,47 @@ def test_post():
 
     result = testdir.runpytest()
     result.assert_outcomes(passed=1, failed=1)
+
+
+# See the thread starting at
+# https://github.com/python-trio/pytest-trio/pull/77#issuecomment-499979536
+# for details on the real case that this was minimized from
+def test_complex_cancel_interaction_regression(testdir):
+    testdir.makepyfile(
+        """
+        import pytest
+        import trio
+        from async_generator import asynccontextmanager, async_generator, yield_
+
+        async def die_soon():
+            raise RuntimeError('oops'.upper())
+
+        @asynccontextmanager
+        @async_generator
+        async def async_finalizer():
+            try:
+                await yield_()
+            finally:
+                await trio.sleep(0)
+
+        @pytest.fixture
+        @async_generator
+        async def fixture(nursery):
+            async with trio.open_nursery() as nursery1:
+                async with async_finalizer():
+                    async with trio.open_nursery() as nursery2:
+                        nursery2.start_soon(die_soon)
+                        await yield_()
+                        nursery1.cancel_scope.cancel()
+
+        @pytest.mark.trio
+        async def test_try(fixture):
+            await trio.sleep_forever()
+        """
+    )
+
+    result = testdir.runpytest()
+    result.assert_outcomes(passed=0, failed=1)
+    result.stdout.fnmatch_lines_random([
+        "*OOPS*",
+    ])

pytest_trio/plugin.py

@@ -4,6 +4,7 @@
 from collections.abc import Coroutine, Generator
 from inspect import iscoroutinefunction, isgeneratorfunction
 import contextvars
+import outcome
 import pytest
 import trio
 from trio.testing import MockClock, trio_test
@@ -132,11 +133,16 @@ class TrioTestContext:
     def __init__(self):
         self.crashed = False
         self.test_cancel_scope = None
+        self.fixtures_with_errors = set()
+        self.fixtures_with_cancel = set()
         self.error_list = []
 
-    def crash(self, exc):
-        if exc is not None:
+    def crash(self, fixture, exc):
+        if exc is None:
+            self.fixtures_with_cancel.add(fixture)
+        else:
             self.error_list.append(exc)
+            self.fixtures_with_errors.add(fixture)
         self.crashed = True
         if self.test_cancel_scope is not None:
             self.test_cancel_scope.cancel()
@@ -192,7 +198,7 @@ async def _fixture_manager(self, test_ctx):
                 finally:
                     nursery_fixture.cancel_scope.cancel()
         except BaseException as exc:
-            test_ctx.crash(exc)
+            test_ctx.crash(self, exc)
         finally:
             self.setup_done.set()
             self._teardown_done.set()
@@ -278,27 +284,29 @@ async def run(self, test_ctx, contextvars_ctx):
             # code will get it again if it matters), and then use a shield to
             # keep waiting for the teardown to finish without having to worry
             # about cancellation.
+            yield_outcome = outcome.Value(None)
             try:
                 for event in self.user_done_events:
                     await event.wait()
             except BaseException as exc:
                 assert isinstance(exc, trio.Cancelled)
-                test_ctx.crash(None)
+                yield_outcome = outcome.Error(exc)
+                test_ctx.crash(self, None)
                 with trio.CancelScope(shield=True):
                     for event in self.user_done_events:
                         await event.wait()
 
             # Do our teardown
             if isasyncgen(func_value):
                 try:
-                    await func_value.asend(None)
+                    await yield_outcome.asend(func_value)
                 except StopAsyncIteration:
                     pass
                 else:
                     raise RuntimeError("too many yields in fixture")
             elif isinstance(func_value, Generator):
                 try:
-                    func_value.send(None)
+                    yield_outcome.send(func_value)
                 except StopIteration:
                     pass
                 else:
@@ -339,6 +347,18 @@ async def _bootstrap_fixtures_and_run_test(**kwargs):
                     fixture.run, test_ctx, contextvars_ctx, name=fixture.name
                 )
 
+        silent_cancellers = (
+            test_ctx.fixtures_with_cancel - test_ctx.fixtures_with_errors
+        )
+        if silent_cancellers:
+            for fixture in silent_cancellers:
+                test_ctx.error_list.append(
+                    RuntimeError(
+                        "{} cancelled the test but didn't "
+                        "raise an error".format(fixture.name)
+                    )
+                )
+
         if test_ctx.error_list:
             raise trio.MultiError(test_ctx.error_list)
 

setup.py

@@ -18,6 +18,7 @@
     install_requires=[
         "trio >= 0.11",
         "async_generator >= 1.9",
+        "outcome",
         # For node.get_closest_marker
         "pytest >= 3.6"
     ],