Skip to content

Conversation

@bhearsum
Copy link
Contributor

@bhearsum bhearsum commented Oct 24, 2025

Also update the comment around this with more details, so I don't get confused next time...

@bhearsum bhearsum force-pushed the push-yorpxmvknkkq branch 2 times, most recently from 7f4d1a7 to 6a308f9 Compare October 24, 2025 19:07
@bhearsum bhearsum marked this pull request as ready for review October 24, 2025 19:13
@bhearsum bhearsum requested a review from a team as a code owner October 24, 2025 19:13
@bhearsum bhearsum force-pushed the push-yorpxmvknkkq branch 2 times, most recently from fb8d0b8 to 9391796 Compare October 28, 2025 00:39
@bhearsum bhearsum changed the title fix: use multiprocessing whenever fork context is available fix: use multiprocessing when Linux and fork context are both available Oct 28, 2025
# processing.
if (
platform.system() != "Linux"
or "fork" not in multiprocessing.get_all_start_methods()
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm still a bit confused. Is fork ever not available on linux?

The commit message / PR title say "fix" but it's not clear what this fixes.

Copy link
Contributor Author

@bhearsum bhearsum Oct 28, 2025

Choose a reason for hiding this comment

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

I'm still a bit confused. Is fork ever not available on linux?

It certainly sounds like it should be; after writing the test I got to thinking that it's probably better to be cautious though? It probably doesn't matter in the real world, at least at this time. (I could be convinced to leave this part alone.)

The commit message / PR title say "fix" but it's not clear what this fixes.

I'll update this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is fork ever not available on linux?

Technically you could make it non available but then the condition wouldn't catch it because python doesn't check if it can fork or not (and honestly, it's probable that python wouldn't even start without fork available on linux).

I got to thinking that it's probably better to be cautious though

I don't think it's worth making the condition more confusing than it need to be for a reader. If I see this without context it's going to make me think that somehow fork could be unavailable and probably overthink it...

FTR python does something like this to populate the start methods (I left the comment because it's interesting for the macos case where you were seeing crashes):

if sys.platform != 'win32':
    _concrete_contexts = {
        'fork': ForkContext(),
        'spawn': SpawnContext(),
        'forkserver': ForkServerContext(),
    }
    # bpo-33725: running arbitrary code after fork() is no longer reliable
    # on macOS since macOS 10.14 (Mojave). Use spawn by default instead.
    # gh-84559: We changed everyones default to a thread safeish one in 3.14.
    if reduction.HAVE_SEND_HANDLE and sys.platform != 'darwin':
        _default_context = DefaultContext(_concrete_contexts['forkserver'])
    else:
        _default_context = DefaultContext(_concrete_contexts['spawn'])
else:  # Windows
    _concrete_contexts = {
      'spawn': SpawnContext(),
    }
    _default_context = DefaultContext(_concrete_contexts['spawn'])

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, I'm convinced!

@bhearsum bhearsum changed the title fix: use multiprocessing when Linux and fork context are both available refactor: use multiprocessing when Linux and fork context are both available Oct 28, 2025
@bhearsum bhearsum changed the title refactor: use multiprocessing when Linux and fork context are both available tests: mark test that depends on multiprocess as Linux-only Oct 28, 2025
Also update the comment around this with more details, so I don't get confused next time...
@bhearsum bhearsum merged commit 773f5d2 into taskcluster:main Oct 29, 2025
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants