From 78289477002251a5b4b26108c0cc407183ee1d38 Mon Sep 17 00:00:00 2001 From: Maxim Nikulin Date: Tue, 21 Apr 2020 21:52:13 +0700 Subject: [PATCH 1/7] Test exceptions in hookwrapper post stage (#244) --- testing/test_multicall.py | 76 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 76 insertions(+) diff --git a/testing/test_multicall.py b/testing/test_multicall.py index 095c7ba2..2345d36f 100644 --- a/testing/test_multicall.py +++ b/testing/test_multicall.py @@ -151,3 +151,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", + ] From 66f13dd0444fc35c2b888c811e2d805f9acafad3 Mon Sep 17 00:00:00 2001 From: Maxim Nikulin Date: Wed, 29 Apr 2020 23:40:17 +0700 Subject: [PATCH 2/7] Hookwrapper finalizer test with many yield Demonstrate that explicit Generator.close() could suppress undefined behavior in respect to the moment when generator finalizer is called. Generator could resist attempts to close it, so one test is marked as xfail. --- testing/test_multicall.py | 45 ++++++++++++++++++++++++++++++++++++--- 1 file changed, 42 insertions(+), 3 deletions(-) diff --git a/testing/test_multicall.py b/testing/test_multicall.py index 2345d36f..83163780 100644 --- a/testing/test_multicall.py +++ b/testing/test_multicall.py @@ -123,15 +123,54 @@ def m1(): def test_hookwrapper_too_many_yield() -> None: + out = [] + @hookimpl(hookwrapper=True) def m1(): - yield 1 - yield 2 + try: + yield 1 + yield 2 + finally: + out.append("cleanup") with pytest.raises(RuntimeError) as ex: - MC([m1], {}) + 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]) From e042809ffc7aedfbfbbb41b97904acd96bc0edcc Mon Sep 17 00:00:00 2001 From: Maxim Nikulin Date: Tue, 12 May 2020 22:58:44 +0700 Subject: [PATCH 3/7] Test for never yield generator wrapper Previously only a case of non-generator hook wrapper was tested. --- testing/test_multicall.py | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/testing/test_multicall.py b/testing/test_multicall.py index 83163780..f222d180 100644 --- a/testing/test_multicall.py +++ b/testing/test_multicall.py @@ -122,6 +122,18 @@ def m1(): MC([m1], {}) +def test_hookwrapper_yield_not_executed() -> None: + @hookimpl(hookwrapper=True) + def m1(): + 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 = [] From fbfb60ced3ba7b52ca1d7c3885f7cffdf45d562b Mon Sep 17 00:00:00 2001 From: Maxim Nikulin Date: Tue, 21 Apr 2020 21:53:16 +0700 Subject: [PATCH 4/7] Handle exceptions in hookwrapper post stage (#244) --- src/pluggy/_callers.py | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/pluggy/_callers.py b/src/pluggy/_callers.py index fbcade23..4b454d4f 100644 --- a/src/pluggy/_callers.py +++ b/src/pluggy/_callers.py @@ -72,8 +72,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() From f7158281ab1b71c64416330a7be91a118209b218 Mon Sep 17 00:00:00 2001 From: Maxim Nikulin Date: Tue, 30 Jun 2020 14:20:35 +0700 Subject: [PATCH 5/7] Limit try-except scope in hookwrapper caller --- src/pluggy/_callers.py | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/src/pluggy/_callers.py b/src/pluggy/_callers.py index 4b454d4f..9837077d 100644 --- a/src/pluggy/_callers.py +++ b/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: From b2edd224e6d37af5110ae64f8f503148eda83cca Mon Sep 17 00:00:00 2001 From: Maxim Nikulin Date: Tue, 30 Jun 2020 19:50:01 +0700 Subject: [PATCH 6/7] Add a changelog note for teardown exceptions --- changelog/244.bugfix.rst | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 changelog/244.bugfix.rst diff --git a/changelog/244.bugfix.rst b/changelog/244.bugfix.rst new file mode 100644 index 00000000..67a900c9 --- /dev/null +++ b/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. From 9532856ba38be322809c2992eda6631180f29e3b Mon Sep 17 00:00:00 2001 From: Maxim Nikulin Date: Tue, 30 Jun 2020 19:51:37 +0700 Subject: [PATCH 7/7] Note on try-finally for wrappers in docs --- docs/index.rst | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/docs/index.rst b/docs/index.rst index a58e9af4..7aec9879 100644 --- a/docs/index.rst +++ b/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 `. +``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.