From f2086249c9453be835f6a431b9a2212d4801603c Mon Sep 17 00:00:00 2001 From: Stephen Finucane Date: Wed, 14 Dec 2022 23:52:06 +0000 Subject: [PATCH 1/3] Do not allow exceptions to propagate from backend The 'pyproject_api' project provides an implementation of a PEP-517 build frontend. Consider the architecture described in the proposal [1]: +-----------+ +---------------+ +----------------+ | frontend | -spawn-> | child cmdline | -Python-> | backend | | | | interface | | implementation | +-----------+ +---------------+ +----------------+ We can map the these components to 'pyproject_api' components like so: frontend -> pyproject_api._frontend.Frontend (or rather, a concrete implementation of same) child cmdline implementation -> pyproject_api._backend backend implementation -> (the backend defined by the '[build-system] build-backend' key in 'pyproject.yaml', typically as parsed by the 'pyproject_api._frontend.Frontend.create_args_from_folder' method) In both the 'pyproject_api._via_fresh_subprocess.SubprocessFrontend' and 'tox.tox_env.python.virtual_env.package.pyproject.Pep517VirtualEnvFrontend' concrete implementations of 'pyproject_api._frontend.Frontend', the child cmdline implementation provided by 'pyproject_api._backend' is called via subprocess. However, there is a crucial difference: while the former will start a new subprocess for each command invoked, the latter will reuse the backend. This can be problematic. Communication between the frontends and the child cmdline implementation is done by issuing commands in serialized JSON format to the 'pyproject_api._backend' executable via stdin, then waiting for a special "Backend: Wrote response foo to $file" message to appear on stdout, before finally reading the contents of the (JSON-formatted) file at '$file'. In the success case, the file at '$file' will contain a single key, 'result', while in the error case the file will contain three keys, 'code', 'exc_type' and 'exc_msg'. The contents of the latter keys are generated by catching any exception raised by the build backend and processing it. However - and here is the crux of the issue - if these exceptions are not derived from 'Exception' then we allow them to bubble up, killing the backend process in the process. A comment inline suggests this is intended to allow things like 'SystemExit' or 'KeyboardInterrupt', but setuptools' (or rather, distutils') exceptions do not derive from 'Exception' either. This is an issue for frontends that re-use backends like the 'Pep517VirtualEnvFrontend' provided by tox. These backends expect to politely cleanup when the backend fails (as indicated by the presence of 'code', 'exc_type', and 'exc_msg' keys in the result file) by calling a special '_exit' command on the backend. However, if the backend process has died, there's nothing left to answer this request. This causes tox at least to hang forever. There are a two solutions to this. Firstly, we can remove the special case for these two exceptions thus allowing the frontend to manage the lifecycle of the '_backend' executable process regardless. An alternative option would be to add a new key to the result file indicating the backend process has died and the frontend should not expect to be able to do any cleanup, but this would require changing the "interface" defined by this result file and updating all frontend implementations to support this. In this change, we opt for the former since it's simpler and a well behaved frontend should ensure a backend process is never allowed to run forever. [*] Note that this is different to errors the occur while initially starting the backend executable or processing a command. If the backend fails to start, a separate special message is issued ("failed to start backend"), which the frontend is expected to handle, and the exception is re-raised. Invalid requests, meanwhile, are simply ignored and a message is logged which the frontend could choose to handle. [1] https://peps.python.org/pep-0517/#appendix-a-comparison-to-pep-516 Signed-off-by: Stephen Finucane Closes: #39 --- src/pyproject_api/_backend.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/pyproject_api/_backend.py b/src/pyproject_api/_backend.py index 393a738..f0be631 100644 --- a/src/pyproject_api/_backend.py +++ b/src/pyproject_api/_backend.py @@ -97,8 +97,6 @@ def run(argv): result["exc_msg"] = str(exception) if not isinstance(exception, MissingCommand): # for missing command do not print stack traceback.print_exc() - if not isinstance(exception, Exception): # allow SystemExit/KeyboardInterrupt to go through - raise finally: try: with open(result_file, "w") as file_handler: From 94bbf125ef4b3c26ae608db9a3e817d346f1bd2c Mon Sep 17 00:00:00 2001 From: Stephen Finucane Date: Sat, 31 Dec 2022 12:53:48 +0000 Subject: [PATCH 2/3] Add tests for #39 Add some simple-ish tests to prevent regressions. While we're here, we correct a typo in a test filename. Signed-off-by: Stephen Finucane Related: #39 --- tests/test_backend.py | 123 ++++++++++++++++++++ tests/{test_fronted.py => test_frontend.py} | 0 whitelist.txt | 2 + 3 files changed, 125 insertions(+) create mode 100644 tests/test_backend.py rename tests/{test_fronted.py => test_frontend.py} (100%) diff --git a/tests/test_backend.py b/tests/test_backend.py new file mode 100644 index 0000000..57bc6b1 --- /dev/null +++ b/tests/test_backend.py @@ -0,0 +1,123 @@ +from __future__ import annotations + +import json + +import pytest + +from pyproject_api._backend import BackendProxy, run + + +def test_invalid_module(capsys): + with pytest.raises(ImportError): + run([str(False), "an.invalid.module"]) + + captured = capsys.readouterr() + assert "failed to start backend" in captured.err + + +def test_invalid_request(mocker, capsys): + """Validate behavior when an invalid request is issued.""" + command = "invalid json" + + backend_proxy = mocker.MagicMock(spec=BackendProxy) + backend_proxy.return_value = "dummy_result" + backend_proxy.__str__.return_value = "FakeBackendProxy" + mocker.patch("pyproject_api._backend.BackendProxy", return_value=backend_proxy) + mocker.patch("pyproject_api._backend.read_line", return_value=bytearray(command, "utf-8")) + + ret = run([str(False), "a.dummy.module"]) + + assert ret == 0 + captured = capsys.readouterr() + assert "started backend " in captured.out + assert "Backend: incorrect request to backend: " in captured.err + + +def test_exception(mocker, capsys, tmp_path): + """Ensure an exception in the backend is not bubbled up.""" + result = str(tmp_path / "result") + command = json.dumps({"cmd": "dummy_command", "kwargs": {"foo": "bar"}, "result": result}) + + backend_proxy = mocker.MagicMock(spec=BackendProxy) + backend_proxy.side_effect = SystemExit(1) + backend_proxy.__str__.return_value = "FakeBackendProxy" + mocker.patch("pyproject_api._backend.BackendProxy", return_value=backend_proxy) + mocker.patch("pyproject_api._backend.read_line", return_value=bytearray(command, "utf-8")) + + ret = run([str(False), "a.dummy.module"]) + + # We still return 0 and write a result file. The exception should *not* bubble up + assert ret == 0 + captured = capsys.readouterr() + assert "started backend FakeBackendProxy" in captured.out + assert "Backend: run command dummy_command with args {'foo': 'bar'}" in captured.out + assert "Backend: Wrote response " in captured.out + assert "SystemExit: 1" in captured.err + + +def test_valid_request(mocker, capsys, tmp_path): + """Validate the "success" path.""" + result = str(tmp_path / "result") + command = json.dumps({"cmd": "dummy_command", "kwargs": {"foo": "bar"}, "result": result}) + + backend_proxy = mocker.MagicMock(spec=BackendProxy) + backend_proxy.return_value = "dummy-result" + backend_proxy.__str__.return_value = "FakeBackendProxy" + mocker.patch("pyproject_api._backend.BackendProxy", return_value=backend_proxy) + mocker.patch("pyproject_api._backend.read_line", return_value=bytearray(command, "utf-8")) + + ret = run([str(False), "a.dummy.module"]) + + assert ret == 0 + captured = capsys.readouterr() + assert "started backend FakeBackendProxy" in captured.out + assert "Backend: run command dummy_command with args {'foo': 'bar'}" in captured.out + assert "Backend: Wrote response " in captured.out + assert "" == captured.err + + +def test_reuse_process(mocker, capsys, tmp_path): + """Validate behavior when reusing the backend proxy process. + + There are a couple of things we'd like to check here: + + - Ensure we can actually reuse the process. + - Ensure an exception in a call to the backend does not affect subsequent calls. + - Ensure we can exit safely by calling the '_exit' command. + """ + results = [ + str(tmp_path / "result_a"), + str(tmp_path / "result_b"), + str(tmp_path / "result_c"), + str(tmp_path / "result_d"), + ] + commands = [ + json.dumps({"cmd": "dummy_command_a", "kwargs": {"foo": "bar"}, "result": results[0]}), + json.dumps({"cmd": "dummy_command_b", "kwargs": {"baz": "qux"}, "result": results[1]}), + json.dumps({"cmd": "dummy_command_c", "kwargs": {"win": "wow"}, "result": results[2]}), + json.dumps({"cmd": "_exit", "kwargs": {}, "result": results[3]}), + ] + + def fake_backend(name, *args, **kwargs): # noqa: U100 + if name == "dummy_command_b": + raise SystemExit(2) + + return "dummy-result" + + backend_proxy = mocker.MagicMock(spec=BackendProxy) + backend_proxy.side_effect = fake_backend + backend_proxy.__str__.return_value = "FakeBackendProxy" + mocker.patch("pyproject_api._backend.BackendProxy", return_value=backend_proxy) + mocker.patch("pyproject_api._backend.read_line", side_effect=[bytearray(x, "utf-8") for x in commands]) + + ret = run([str(True), "a.dummy.module"]) + + # We still return 0 and write a result file. The exception should *not* bubble up and all commands should execute. + # It is the responsibility of the caller to handle errors. + assert ret == 0 + captured = capsys.readouterr() + assert "started backend FakeBackendProxy" in captured.out + assert "Backend: run command dummy_command_a with args {'foo': 'bar'}" in captured.out + assert "Backend: run command dummy_command_b with args {'baz': 'qux'}" in captured.out + assert "Backend: run command dummy_command_c with args {'win': 'wow'}" in captured.out + assert "SystemExit: 2" in captured.err diff --git a/tests/test_fronted.py b/tests/test_frontend.py similarity index 100% rename from tests/test_fronted.py rename to tests/test_frontend.py diff --git a/whitelist.txt b/whitelist.txt index 6b04263..284dd83 100644 --- a/whitelist.txt +++ b/whitelist.txt @@ -1,5 +1,6 @@ autoclass autodoc +capsys cfg delenv exe @@ -17,6 +18,7 @@ py311 py38 pygments pyproject +readouterr sdist setenv tmpdir From 8ae2cec66690c8dcc9161d02ab87099636a5ccd7 Mon Sep 17 00:00:00 2001 From: Stephen Finucane Date: Tue, 3 Jan 2023 11:12:12 +0000 Subject: [PATCH 3/3] Add missing types to new tests Signed-off-by: Stephen Finucane --- tests/test_backend.py | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/tests/test_backend.py b/tests/test_backend.py index 57bc6b1..4b682b3 100644 --- a/tests/test_backend.py +++ b/tests/test_backend.py @@ -1,13 +1,16 @@ from __future__ import annotations import json +from pathlib import Path +from typing import Any import pytest +import pytest_mock from pyproject_api._backend import BackendProxy, run -def test_invalid_module(capsys): +def test_invalid_module(capsys: pytest.CaptureFixture[str]) -> None: with pytest.raises(ImportError): run([str(False), "an.invalid.module"]) @@ -15,7 +18,7 @@ def test_invalid_module(capsys): assert "failed to start backend" in captured.err -def test_invalid_request(mocker, capsys): +def test_invalid_request(mocker: pytest_mock.MockerFixture, capsys: pytest.CaptureFixture[str]) -> None: """Validate behavior when an invalid request is issued.""" command = "invalid json" @@ -33,7 +36,7 @@ def test_invalid_request(mocker, capsys): assert "Backend: incorrect request to backend: " in captured.err -def test_exception(mocker, capsys, tmp_path): +def test_exception(mocker: pytest_mock.MockerFixture, capsys: pytest.CaptureFixture[str], tmp_path: Path) -> None: """Ensure an exception in the backend is not bubbled up.""" result = str(tmp_path / "result") command = json.dumps({"cmd": "dummy_command", "kwargs": {"foo": "bar"}, "result": result}) @@ -55,7 +58,7 @@ def test_exception(mocker, capsys, tmp_path): assert "SystemExit: 1" in captured.err -def test_valid_request(mocker, capsys, tmp_path): +def test_valid_request(mocker: pytest_mock.MockerFixture, capsys: pytest.CaptureFixture[str], tmp_path: Path) -> None: """Validate the "success" path.""" result = str(tmp_path / "result") command = json.dumps({"cmd": "dummy_command", "kwargs": {"foo": "bar"}, "result": result}) @@ -76,7 +79,7 @@ def test_valid_request(mocker, capsys, tmp_path): assert "" == captured.err -def test_reuse_process(mocker, capsys, tmp_path): +def test_reuse_process(mocker: pytest_mock.MockerFixture, capsys: pytest.CaptureFixture[str], tmp_path: Path) -> None: """Validate behavior when reusing the backend proxy process. There are a couple of things we'd like to check here: @@ -98,7 +101,7 @@ def test_reuse_process(mocker, capsys, tmp_path): json.dumps({"cmd": "_exit", "kwargs": {}, "result": results[3]}), ] - def fake_backend(name, *args, **kwargs): # noqa: U100 + def fake_backend(name: str, *args: Any, **kwargs: Any) -> Any: # noqa: U100 if name == "dummy_command_b": raise SystemExit(2)