Use forkserver start method for multiprocessing #296
Conversation
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
eltoder
force-pushed
the
feature/forkserver
branch
from
89064f3
to
faaa288
Compare
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) |
There was a problem hiding this comment.
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.
| try: | ||
| return _main(argv, testing) | ||
| finally: | ||
| del os.environ["TMPDIR"] |
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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.)
|
Note: GH actions promoted @CleanCut FYI |
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: