Skip to content

Use multiprocessing start_method "forkserver" #4306

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Nov 23, 2022
Merged

Use multiprocessing start_method "forkserver" #4306

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0c38251
Use `multiprocessing` `start_method` `"forkserver"`
rwgk Nov 1, 2022
0b089f3
Add link to comment under PR #4105
rwgk Nov 2, 2022
2bbea88
Unconditionally `pytest.skip("DEADLOCK")` for PyPy Windows
rwgk Nov 2, 2022
29cea64
Remove `SKIP_IF_DEADLOCK` entirely, for simplicity. Hopefully this PR…
rwgk Nov 23, 2022
74bbc04
Add "In a nutshell" comment, in response to request by @EricCousineau…
rwgk Nov 23, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions tests/conftest.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@
import contextlib
import difflib
import gc
import multiprocessing
import os
import re
import textwrap

Expand All @@ -15,6 +17,16 @@
# Early diagnostic for failed imports
import pybind11_tests

if os.name != "nt":
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit Can you add a comment as to why we're doing this here?
git blame is decent for more context, but I think two small sentences can help explain, e.g.:

# We want to shy away from Python's default choice for Linux of "fork", as it can incur
# post-fork resource contention issues (such as thread ids, etc.).

I'm vaguely familiar with this, and with our codebase we hit it with either sledgehammers of:

  • "yes fork, but pin down threading for fast-but-reckless multiprocessing spawns"
  • "no fork, please use spawn, and don't think about it" (I still need to read about forkserver and how it may interact with imports like numpy or cv2 that may configure / spawn threads at import)
    EricCousineau-TRI/repro@2cff386

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a comment as to why we're doing this here?

Done. I tried to keep it concise.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm... While this and the issue describes the symptoms, is there any way you can link to something that provides readers more context about the root cause of why threads + fork == bad / flaky?
(feel free to dismiss if you wanna add to issue discussion itself, or punt)

# 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")
_hexadecimal = re.compile(r"0x[0-9a-fA-F]+")

Expand Down
4 changes: 2 additions & 2 deletions tests/test_gil_scoped.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

import pytest

import env
from pybind11_tests import gil_scoped as m


Expand Down Expand Up @@ -144,7 +145,6 @@ def _intentional_deadlock():


ALL_BASIC_TESTS_PLUS_INTENTIONAL_DEADLOCK = ALL_BASIC_TESTS + (_intentional_deadlock,)
SKIP_IF_DEADLOCK = True # See PR #4216


def _run_in_process(target, *args, **kwargs):
Expand Down Expand Up @@ -181,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:
if env.PYPY and env.WIN:
pytest.skip(msg)
raise RuntimeError(msg)
return process.exitcode
Expand Down