Skip to content
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.

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

Use forkserver start method for multiprocessing #296

Merged
merged 2 commits into from
Apr 24, 2024

Conversation

eltoder
Copy link
Contributor

@eltoder eltoder commented Apr 19, 2024

Use forkserver when available to avoid issues with forking multi-threaded processes. See python/cpython#84559 for more context.

This also removes a warning when running on python 3.12 than can be seen in CI:

/opt/hostedtoolcache/Python/3.12.2/x64/lib/python3.12/multiprocessing/popen_fork.py:66: DeprecationWarning: This process (pid=1991) is multi-threaded, use of fork() may lead to deadlocks in the child.
self.pid = os.fork()

@coveralls
Copy link

coveralls commented Apr 19, 2024

Coverage Status

coverage: 95.232% (+0.2%) from 95.014%
when pulling 7899692 on eltoder:feature/forkserver
into 4de285b on CleanCut:main.

Use forkserver when available to avoid issues with forking
multi-threaded processes. See [1] for more context.

This also removes a warning when running on python 3.12 that can be seen
in CI:

> /opt/hostedtoolcache/Python/3.12.2/x64/lib/python3.12/multiprocessing/popen_fork.py:66: DeprecationWarning: This process (pid=1991) is multi-threaded, use of fork() may lead to deadlocks in the child.
>   self.pid = os.fork()

[1] python/cpython#84559
@sodul sodul self-assigned this Apr 23, 2024
green/cmdline.py Outdated
atexit.register(lambda: shutil.rmtree(temp_dir_for_tests, ignore_errors=True))
os.environ["TMPDIR"] = temp_dir_for_tests
tempfile.tempdir = temp_dir_for_tests
return _main(argv, testing)
Copy link
Collaborator

@sodul sodul Apr 23, 2024

Choose a reason for hiding this comment

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

Compared to the original code there is a little less cleanup logic here. For example the if block is setting tempfile.tempdir and os.environ["TMPDIR"]. The original code would reset them but the new code is not doing that. I understand that main() is going to be called mostly from the if __name__ == "__main__": block but it is possible that someone in the wild is calling green.cmdline.main() from a loop and in such case we are not properly resetting the values.

I agree that the atexit.register() is an improvement but we should not skip the cleanup if the interpreter is not exiting yet.

Something we should probably re-add here:
Track the prior values for TMPDIR and tempfile.tempdir and always reset to that if we changed them. This can be done be re-adding a return _main(argv, testing) wrapped by a try/finally in the if block.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Collaborator

@sodul sodul left a comment

Choose a reason for hiding this comment

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

Minor concerns in cmdline.py, the rest of the changes look good to me.

@sodul sodul requested a review from CleanCut April 23, 2024 21:22
@eltoder eltoder requested a review from sodul April 24, 2024 00:38
Copy link
Collaborator

@sodul sodul left a comment

Choose a reason for hiding this comment

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

LGTM

try:
return _main(argv, testing)
finally:
del os.environ["TMPDIR"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ideally we would want reset TMPDIR to the previous version if any. Since it was the pre-existing behavior I'm approving since the changes are definitely a net improvement.

Thank you!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We checked that TMPDIR was not set before entering this if branch. (Technically, it was either unset or set to an empty string, but these are the same for python's tempfile logic, so I think it's fine to delete in either case.)

@sodul sodul merged commit 8cabab8 into CleanCut:main Apr 24, 2024
25 checks passed
@eltoder eltoder deleted the feature/forkserver branch April 24, 2024 16:40
@sodul
Copy link
Collaborator

sodul commented Apr 24, 2024

Note: GH actions promoted macos-latest to newer machines with ARM cpus. Unfortunately the GH actions that try to run with python 3.8 and 3.9 broke overnight impacting many OSS projects. This mean that we will either need to update our actions with a temporary workaround or wait for the maintainers to provider a permanent fix before we can perform new releases.

actions/setup-python#850

@CleanCut FYI

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