-
-
Notifications
You must be signed in to change notification settings - Fork 33.6k
gh-142302: Fix mkstemp() documentation: clarify file descriptor inheritance behavior #142338
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
base: main
Are you sure you want to change the base?
Conversation
The documentation incorrectly stated that the file descriptor is not inherited by child processes. In reality, the close-on-exec flag (when available) only prevents inheritance across exec() calls, not fork(). The fd will still be inherited by forked child processes.
|
Please sign the CLA and address the CI issues. In addition, please wait for the maintainer to confirm that the documentation should be changed like that. |
Adjust indentation to match the rest of the file formatting.
|
Thank you for your review
|
|
Hopefully this is appropriate to comment here - I don't see that O_CLOEXEC is included as a flag in the tempfile.mkstemp: Following things backward from where the file descriptor is created here: It looks like the flags are created here: Which is just a simple switch to these guys: I have only tested this on "fork" but theoretically the file descriptor will be inherited across the "exec" calls as well since the O_CLOEXEC doesn't appear to be present in any of the flags during the file descriptor creation, and the tempfile.mkstemp function signature does not present the user any options to add O_CLOEXEC or any other filesystem-level flags (presumably by design, since the user could use their own calls to os.open if they needed that fine-grained flag control). |
…w , O_CLOEXEC is not included in the flags used by mkstemp(). The fd will be inherited by all child processes. Thanks to @QuiteAFoxtrot for the investigation
|
@QuiteAFoxtrot You're absolutely right. I've confirmed that _text_openflags = _os.O_RDWR | _os.O_CREAT | _os.O_EXCL
_bin_openflags = _text_openflagsThis means that file descriptor will be inherited by the child processes created via both |
|
@vstinner 's comments on the original issue #142302 show that O_CLOEXEC is part of the default behavior of the call to |
vstinner
left a comment
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.
This documentation change is wrong, I suggest to close it.
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
|
Respectfully, I'd like to point out that the original docs were also wrong - the file descriptor is inherited (and more importantly, usable) with a fork()-no-exec(), shouldn't the docs still be fixed? Apologies if that's not what you meant by suggesting to close. |
|
If you consider that the documentation is unclear, you can complete/clarify https://docs.python.org/dev/library/os.html#fd-inheritance. And you might add a link to this section from mkstemp() doc ("The file descriptor is not inherited by child processes"). |
The original documentation stated 'The file descriptor is not inherited by child processes' without clarifying that this only applies to exec() calls due to O_CLOEXEC (set via PEP 446). The fd is still inherited and usable after fork() before exec(). Update documentation to specifically mention exec() calls for accuracy
|
Thank you @vstinner and @QuiteAFoxtrot for the clarification regarding PEP 446 and O_CLOEXEC behaviour.
|
|
Thanks for making the requested changes! @vstinner: please review the changes made to this pull request. |
| @@ -0,0 +1,477 @@ | |||
| ============================================================= | |||
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.
Please remove this file.
| the file is executable by no one. | ||
|
|
||
| The file descriptor is not inherited by child processes across | ||
| :func:`exec <os.execl>` calls. |
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.
I suggested to add a link to https://docs.python.org/dev/library/os.html#fd-inheritance and clarify https://docs.python.org/dev/library/os.html#fd-inheritance about fork vs exec.
Description
Fixes the misleading documentation for
tempfile.mkstemp()regarding file descriptor inheritance.The current documentation states "The file descriptor is not inherited by child processes," which is incomplete and misleading.
What was wrong
The file descriptor created by
mkstemp()is inherited by child processes created viafork(). The close-on-exec flag (O_CLOEXEC), when available, only prevents inheritance acrossexec()calls, notfork()calls.Changes made
Updated the documentation to accurately describe the inheritance behavior:
O_CLOEXEConly affectsexec()callsTesting
Verified the documentation builds correctly:
cd Doc make htmlRelated Issue
Fixes #142302
📚 Documentation preview 📚: https://cpython-previews--142338.org.readthedocs.build/