Handle exceptions in hookwrapper post stage (#244)

changelog/244.bugfix.rst

@@ -0,0 +1,6 @@
+Exceptions raised by hook wrappers in the post-yield part propagate
+to outer wrappers boxed into ``_Result`` container.
+Previously no other teardown code was run in other hooks after such exception.
+
+* Use ``try-yield-finally`` pattern in hook wrappers intended for setup and teardown.
+* Raise an exception to convert hook result into failure.

docs/index.rst

@@ -402,9 +402,19 @@ the final result(s) returned back to the caller using the
 :py:meth:`~pluggy._callers._Result.force_result` or
 :py:meth:`~pluggy._callers._Result.get_result` methods.
 
+If a wrapper is intended for setup and tear down of other stuff,
+do not forget to surround ``yield`` and ``outcome.get_result()``
+with ``try`` and ``finally`` exactly as it is recommended for
+:py:func:`@contextlib.contextmanager <python:contextlib.contextmanager>`.
+``outcome.get_result()`` could raise an exception if some other hook failed.
+For future compatibility it is better to assume that ``yield`` could
+throw as well.
+
 .. note::
     Hook wrappers can **not** return results (as per generator function
     semantics); they can only modify them using the ``_Result`` API.
+    However an exception following ``yield`` implicitly replaces result
+    for the outer wrappers if there are any of them.
 
 Also see the :ref:`pytest:hookwrapper` section in the ``pytest`` docs.
 

src/pluggy/_callers.py

@@ -39,15 +39,17 @@ def _multicall(
                             )
 
                 if hook_impl.hookwrapper:
+                    # If this cast is not valid, a type error is raised below,
+                    # which is the desired response.
+                    res = hook_impl.function(*args)
+                    gen = cast(Generator[None, _Result[object], None], res)
+
                     try:
-                        # If this cast is not valid, a type error is raised below,
-                        # which is the desired response.
-                        res = hook_impl.function(*args)
-                        gen = cast(Generator[None, _Result[object], None], res)
                         next(gen)  # first yield
-                        teardowns.append(gen)
                     except StopIteration:
                         _raise_wrapfail(gen, "did not yield")
+
+                    teardowns.append(gen)
                 else:
                     res = hook_impl.function(*args)
                     if res is not None:
@@ -72,8 +74,18 @@ def _multicall(
         for gen in reversed(teardowns):
             try:
                 gen.send(outcome)
+                # Following is unreachable for a well behaved hook wrapper.
+                # Try to force finalizers otherwise postponed till GC action.
+                # Note: close() may raise if generator handles GeneratorExit.
+                gen.close()
                 _raise_wrapfail(gen, "has second yield")
             except StopIteration:
+                # Regular code path: exited after single yield, close() is unnecessary.
                 pass
+            except BaseException:
+                # Any other exception: instead of yield, in response to close, extra yield.
+                cur_excinfo = sys.exc_info()
+                if cur_excinfo[0] is not None:  # silence type checker
+                    outcome._excinfo = cur_excinfo
 
         return outcome.get_result()

testing/test_multicall.py

@@ -122,16 +122,67 @@ def m1():
         MC([m1], {})
 
 
-def test_hookwrapper_too_many_yield() -> None:
+def test_hookwrapper_yield_not_executed() -> None:
     @hookimpl(hookwrapper=True)
     def m1():
-        yield 1
-        yield 2
+        if False:
+            yield  # type: ignore[unreachable]
+        return
 
     with pytest.raises(RuntimeError) as ex:
         MC([m1], {})
+    assert "did not yield" in str(ex.value)
+
+
+def test_hookwrapper_too_many_yield() -> None:
+    out = []
+
+    @hookimpl(hookwrapper=True)
+    def m1():
+        try:
+            yield 1
+            yield 2
+        finally:
+            out.append("cleanup")
+
+    with pytest.raises(RuntimeError) as ex:
+        try:
+            MC([m1], {})
+        finally:
+            out.append("finally")
     assert "m1" in str(ex.value)
     assert (__file__ + ":") in str(ex.value)
+    assert out == [
+        "cleanup",
+        "finally",
+    ]
+
+
+@pytest.mark.xfail
+def test_hookwrapper_blocked_generator_exit() -> None:
+    out = []
+
+    @hookimpl(hookwrapper=True)
+    def m1():
+        try:
+            yield 1
+            yield 2
+        except GeneratorExit:
+            yield 3
+        finally:
+            # I have no idea if it is possible to force cleanup in such cases.
+            out.append("unreachable cleanup")
+
+    with pytest.raises(RuntimeError) as ex:
+        try:
+            MC([m1], {})
+        finally:
+            out.append("finally")
+    assert "generator ignored GeneratorExit" in str(ex.value)
+    assert out == [
+        "unreachable cleanup",
+        "finally",
+    ]
 
 
 @pytest.mark.parametrize("exc", [ValueError, SystemExit])
@@ -151,3 +202,79 @@ def m2():
     with pytest.raises(exc):
         MC([m2, m1], {})
     assert out == ["m1 init", "m1 finish"]
+
+
+def test_unwind_inner_wrapper_teardown_exc() -> None:
+    out = []
+
+    @hookimpl(hookwrapper=True)
+    def m1():
+        out.append("m1 init")
+        try:
+            outcome = yield 1
+            out.append("m1 teardown")
+            outcome.get_result()
+            out.append("m1 unreachable")
+        finally:
+            out.append("m1 cleanup")
+
+    @hookimpl(hookwrapper=True)
+    def m2():
+        out.append("m2 init")
+        yield 2
+        out.append("m2 raise")
+        raise ValueError()
+
+    with pytest.raises(ValueError):
+        try:
+            MC([m2, m1], {})
+        finally:
+            out.append("finally")
+
+    assert out == [
+        "m1 init",
+        "m2 init",
+        "m2 raise",
+        "m1 teardown",
+        "m1 cleanup",
+        "finally",
+    ]
+
+
+def test_suppress_inner_wrapper_teardown_exc() -> None:
+    out = []
+
+    @hookimpl(hookwrapper=True)
+    def m1():
+        out.append("m1 init")
+        outcome = yield 1
+        outcome.get_result()
+        out.append("m1 finish")
+
+    @hookimpl(hookwrapper=True)
+    def m2():
+        out.append("m2 init")
+        try:
+            outcome = yield 2
+            outcome.get_result()
+            out.append("m2 unreachable")
+        except ValueError:
+            outcome.force_result(22)
+            out.append("m2 suppress")
+
+    @hookimpl(hookwrapper=True)
+    def m3():
+        out.append("m3 init")
+        yield 3
+        out.append("m3 raise")
+        raise ValueError()
+
+    assert 22 == MC([m3, m2, m1], {})
+    assert out == [
+        "m1 init",
+        "m2 init",
+        "m3 init",
+        "m3 raise",
+        "m2 suppress",
+        "m1 finish",
+    ]