Skip to content

Commit 9391796

Browse files
committed
fix: use multiprocessing when Linux and fork context are both available
Also update the comment around this with more details, so I don't get confused next time...
1 parent b9c95a0 commit 9391796

File tree

2 files changed

+36
-11
lines changed

2 files changed

+36
-11
lines changed

src/taskgraph/generator.py

Lines changed: 24 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -434,14 +434,30 @@ def _run(self):
434434
yield "kind_graph", kind_graph
435435

436436
logger.info("Generating full task set")
437-
# Current parallel generation relies on multiprocessing, and forking.
438-
# This causes problems on Windows and macOS due to how new processes
439-
# are created there, and how doing so reinitializes global variables
440-
# that are modified earlier in graph generation, that doesn't get
441-
# redone in the new processes. Ideally this would be fixed, or we
442-
# would take another approach to parallel kind generation. In the
443-
# meantime, it's not supported outside of Linux.
444-
if platform.system() != "Linux" or os.environ.get("TASKGRAPH_SERIAL"):
437+
# The short version of the below is: we only support parallel kind
438+
# processing on Linux when the `fork` method is available.
439+
#
440+
# Current parallel generation relies on multiprocessing, and more
441+
# specifically: the "fork" multiprocessing method. This is not supported
442+
# at all on Windows (it uses "spawn"). Forking is supported on macOS,
443+
# but no longer works reliably in all cases, and our usage of it here
444+
# causes crashes. See https://github.com/python/cpython/issues/77906
445+
# and http://sealiesoftware.com/blog/archive/2017/6/5/Objective-C_and_fork_in_macOS_1013.html
446+
# for more details on that.
447+
# Other methods of multiprocessing (both "spawn" and "forkserver")
448+
# do not work for our use case, because they cause global variables
449+
# to be reinitialized, which are sometimes modified earlier in graph
450+
# generation. These issues can theoretically be worked around by
451+
# eliminating all reliance on globals as part of task generation, but
452+
# is far from a small amount of work in users like Gecko/Firefox.
453+
# In the long term, the better path forward is likely to be switching
454+
# to threading with a free-threaded python to achieve similar parallel
455+
# processing.
456+
if (
457+
platform.system() != "Linux"
458+
or "fork" not in multiprocessing.get_all_start_methods()
459+
or os.environ.get("TASKGRAPH_SERIAL")
460+
):
445461
all_tasks = self._load_tasks_serial(kinds, kind_graph, parameters)
446462
else:
447463
all_tasks = self._load_tasks_parallel(kinds, kind_graph, parameters)

test/test_generator.py

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,8 @@
33
# file, You can obtain one at http://mozilla.org/MPL/2.0/.
44

55

6+
import multiprocessing
7+
import platform
68
from concurrent.futures import ProcessPoolExecutor
79

810
import pytest
@@ -12,6 +14,12 @@
1214
from taskgraph.generator import Kind, load_tasks_for_kind, load_tasks_for_kinds
1315
from taskgraph.loader.default import loader as default_loader
1416

17+
multiprocessonly = pytest.mark.skipif(
18+
platform.system() != "Linux"
19+
or "fork" not in multiprocessing.get_all_start_methods(),
20+
reason="requires Linux and 'fork' multiprocessing support",
21+
)
22+
1523

1624
class FakePPE(ProcessPoolExecutor):
1725
loaded_kinds = []
@@ -21,6 +29,7 @@ def submit(self, kind_load_tasks, *args):
2129
return super().submit(kind_load_tasks, *args)
2230

2331

32+
@multiprocessonly
2433
def test_kind_ordering(mocker, maketgg):
2534
"When task kinds depend on each other, they are loaded in postorder"
2635
mocked_ppe = mocker.patch.object(generator, "ProcessPoolExecutor", new=FakePPE)
@@ -272,9 +281,9 @@ def _get_loader(self):
272281
)
273282
def test_default_loader(config, expected_transforms):
274283
loader = Kind("", "", config, {})._get_loader()
275-
assert loader is default_loader, (
276-
"Default Kind loader should be taskgraph.loader.default.loader"
277-
)
284+
assert (
285+
loader is default_loader
286+
), "Default Kind loader should be taskgraph.loader.default.loader"
278287
loader("", "", config, {}, [], False)
279288

280289
assert config["transforms"] == expected_transforms

0 commit comments

Comments
 (0)