From 0c382516e0eb8fa827c2ddd8ff2867db7d734123 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Tue, 1 Nov 2022 15:24:16 -0700 Subject: [PATCH 1/5] Use `multiprocessing` `start_method` `"forkserver"` Alternative to PR #4305 --- tests/conftest.py | 5 +++++ tests/test_gil_scoped.py | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/tests/conftest.py b/tests/conftest.py index f5ddb9f129..356893aace 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -7,6 +7,8 @@ import contextlib import difflib import gc +import multiprocessing +import os import re import textwrap @@ -15,6 +17,9 @@ # Early diagnostic for failed imports import pybind11_tests +if os.name != "nt": + multiprocessing.set_start_method("forkserver") + _long_marker = re.compile(r"([0-9])L") _hexadecimal = re.compile(r"0x[0-9a-fA-F]+") diff --git a/tests/test_gil_scoped.py b/tests/test_gil_scoped.py index e890a7b0c8..931e3af213 100644 --- a/tests/test_gil_scoped.py +++ b/tests/test_gil_scoped.py @@ -144,7 +144,7 @@ def _intentional_deadlock(): ALL_BASIC_TESTS_PLUS_INTENTIONAL_DEADLOCK = ALL_BASIC_TESTS + (_intentional_deadlock,) -SKIP_IF_DEADLOCK = True # See PR #4216 +SKIP_IF_DEADLOCK = False # See PR #4216 def _run_in_process(target, *args, **kwargs): From 0b089f39a62642f29c6caaadaab6711ac4048d9e Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Wed, 2 Nov 2022 10:49:42 -0700 Subject: [PATCH 2/5] Add link to comment under PR #4105 --- tests/conftest.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/conftest.py b/tests/conftest.py index 356893aace..69ac3b3a49 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -18,6 +18,7 @@ import pybind11_tests if os.name != "nt": + # https://github.com/pybind/pybind11/issues/4105#issuecomment-1301004592 multiprocessing.set_start_method("forkserver") _long_marker = re.compile(r"([0-9])L") From 2bbea88da13884d18f866bf152b9b714077acc48 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Wed, 2 Nov 2022 11:25:27 -0700 Subject: [PATCH 3/5] Unconditionally `pytest.skip("DEADLOCK")` for PyPy Windows --- tests/test_gil_scoped.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/test_gil_scoped.py b/tests/test_gil_scoped.py index 931e3af213..e0cd5cf7f6 100644 --- a/tests/test_gil_scoped.py +++ b/tests/test_gil_scoped.py @@ -5,6 +5,7 @@ import pytest +import env from pybind11_tests import gil_scoped as m @@ -181,7 +182,7 @@ def _run_in_process(target, *args, **kwargs): elif process.exitcode is None: assert t_delta > 0.9 * timeout msg = "DEADLOCK, most likely, exactly what this test is meant to detect." - if SKIP_IF_DEADLOCK: + if SKIP_IF_DEADLOCK or (env.PYPY and env.WIN): pytest.skip(msg) raise RuntimeError(msg) return process.exitcode From 29cea64554c19d5d7bc412d46c0b39d1c9705bd7 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Tue, 22 Nov 2022 16:16:27 -0800 Subject: [PATCH 4/5] Remove `SKIP_IF_DEADLOCK` entirely, for simplicity. Hopefully this PR will resolve the deadlocks for good. --- tests/test_gil_scoped.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/test_gil_scoped.py b/tests/test_gil_scoped.py index e0cd5cf7f6..6af6a472d5 100644 --- a/tests/test_gil_scoped.py +++ b/tests/test_gil_scoped.py @@ -145,7 +145,6 @@ def _intentional_deadlock(): ALL_BASIC_TESTS_PLUS_INTENTIONAL_DEADLOCK = ALL_BASIC_TESTS + (_intentional_deadlock,) -SKIP_IF_DEADLOCK = False # See PR #4216 def _run_in_process(target, *args, **kwargs): @@ -182,7 +181,7 @@ def _run_in_process(target, *args, **kwargs): elif process.exitcode is None: assert t_delta > 0.9 * timeout msg = "DEADLOCK, most likely, exactly what this test is meant to detect." - if SKIP_IF_DEADLOCK or (env.PYPY and env.WIN): + if env.PYPY and env.WIN: pytest.skip(msg) raise RuntimeError(msg) return process.exitcode From 74bbc04912bdea4f0930ccaaddeb7d3d531a79a0 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Tue, 22 Nov 2022 16:18:08 -0800 Subject: [PATCH 5/5] Add "In a nutshell" comment, in response to request by @EricCousineau-TRI --- tests/conftest.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/tests/conftest.py b/tests/conftest.py index 69ac3b3a49..96dffc81cc 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -18,7 +18,13 @@ import pybind11_tests if os.name != "nt": - # https://github.com/pybind/pybind11/issues/4105#issuecomment-1301004592 + # Full background: https://github.com/pybind/pybind11/issues/4105#issuecomment-1301004592 + # In a nutshell: fork() after starting threads == flakiness in the form of deadlocks. + # It is actually a well-known pitfall, unfortunately without guard rails. + # "forkserver" is more performant than "spawn" (~9s vs ~13s for tests/test_gil_scoped.py, + # visit the issuecomment link above for details). + # Windows does not have fork() and the associated pitfall, therefore it is best left + # running with defaults. multiprocessing.set_start_method("forkserver") _long_marker = re.compile(r"([0-9])L")