-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
#12068: Fix incorrect env handling in spawnProcess on Posix when posix_spawnp is used. #12069
Conversation
e272748
to
76d08da
Compare
please review |
76d08da
to
7821139
Compare
Thanks for the PR! Not sure what to do here. I think that we should fix the documentation instead. That is, the bug lies in the documentation and not in the code :) This change was introduced 2 years ago and is present in a public release. By doing this change, we will break backward compatibility. The initial change from 16 years ago was using My understanding is that execvpe always requires an explicit environment, and it will not copy the environment from the parent process. Regards |
Normally I would agree, except the circumstances under which In this specific case, I think the least impactful thing is to fix the code, rather than the docs. But perhaps we should shoot this over to the mailing list for a tiebreaker? |
7821139
to
a37d7f1
Compare
This change of behavior was actually released in 23.8.0 by enabling use of The regression is actually pretty bad. It silently drops environment variables only under certain conditions (e.g. when This is how the bug was discovered in the first place - by investigating a regression in a downstream project caused by updating Twisted.
The code fix is needed in either case, because if we fix the docs, we need to adjust the behavior of how |
execvpe uses os.environ when env argument is None. See here: https://github.com/python/cpython/blob/3.12/Lib/os.py#L594 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Many thanks, Povilas for the PR and the explanations and investigation.
With the latest info, I also think that is best to fix the code.
I left a few minor comments regarding the scope/purpose of each test.
I tried to read the code, but I can't find why fork is expected to be used in those tests.
Please check my commend and add your feedback.
Other than that, this can be merged.
Once merged, I will try to trigger a new release.
Thanks again.
47a3d83
to
7057f1b
Compare
please review |
5806332
to
83b03ba
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Many thanks for the PR.
It looks good.
My only blocker for this PR is trying to use a dedicated reactor instance, rather than using the global reactor.
Maybe it's a bad suggestion :)
See my inline comments. Thanks!
Documentation on reactor.spawnProcess says the following about env parameter: env is None: On POSIX: pass os.environ This was not correctly handled in os.posix_spawnp() code path.
83b03ba
to
90c56ab
Compare
please review |
Thanks a lot for your suggestions. I think I addressed all comments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great. Thanks for integrating all that feedback.
Fixes #12068. This is a regression starting at Twisted 23.8.0.
Documentation on reactor.spawnProcess says the following about env parameter:
env is None: On POSIX: pass os.environ
This was not correctly handled in
os.posix_spawnp()
code path.Scope and purpose
This PR is just a simple one line fix to address #12068. Tests for environment passing on Posix have been added.