Skip to content

Commit 24b2719

Browse files
committed
tests: mark test that depends on multiprocess as Linux-only
Also update the comment around this with more details, so I don't get confused next time...
1 parent b9c95a0 commit 24b2719

File tree

2 files changed

+30
-10
lines changed

2 files changed

+30
-10
lines changed

src/taskgraph/generator.py

Lines changed: 19 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -434,13 +434,25 @@ 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.
437+
# The short version of the below is: we only support parallel kind
438+
# processing on Linux.
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.
444456
if platform.system() != "Linux" or os.environ.get("TASKGRAPH_SERIAL"):
445457
all_tasks = self._load_tasks_serial(kinds, kind_graph, parameters)
446458
else:

test/test_generator.py

Lines changed: 11 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,11 @@
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+
linuxonly = pytest.mark.skipif(
18+
platform.system() != "Linux",
19+
reason="requires Linux and 'fork' multiprocessing support",
20+
)
21+
1522

1623
class FakePPE(ProcessPoolExecutor):
1724
loaded_kinds = []
@@ -21,6 +28,7 @@ def submit(self, kind_load_tasks, *args):
2128
return super().submit(kind_load_tasks, *args)
2229

2330

31+
@linuxonly
2432
def test_kind_ordering(mocker, maketgg):
2533
"When task kinds depend on each other, they are loaded in postorder"
2634
mocked_ppe = mocker.patch.object(generator, "ProcessPoolExecutor", new=FakePPE)
@@ -272,9 +280,9 @@ def _get_loader(self):
272280
)
273281
def test_default_loader(config, expected_transforms):
274282
loader = Kind("", "", config, {})._get_loader()
275-
assert loader is default_loader, (
276-
"Default Kind loader should be taskgraph.loader.default.loader"
277-
)
283+
assert (
284+
loader is default_loader
285+
), "Default Kind loader should be taskgraph.loader.default.loader"
278286
loader("", "", config, {}, [], False)
279287

280288
assert config["transforms"] == expected_transforms

0 commit comments

Comments
 (0)