bpo-35537: subprocess can use posix_spawn with pipes #11575
Conversation
|
Follow-up of PR #11452. |
| file_actions = [] | ||
| for fd in (p2cwrite, c2pread, errread): | ||
| if fd != -1: | ||
| file_actions.append((os.POSIX_SPAWN_CLOSE, fd)) |
There was a problem hiding this comment.
Technically, there is no need to explicitly close parent pipe ends because they have O_CLOEXEC. One small advantage of doing that is to reduce the number of descriptors before the following dup2, which could increase their number (in a corner case when any of descriptors 0, 1, 2 is closed in the parent process) and potentially hit resource limits.
There was a problem hiding this comment.
Technically, there is no need, but _posixsubprocess does that. In case of doubt, I prefer to mimick _posixsubprocess behavior.
/* Close parent's pipe ends. */
if (p2cwrite != -1)
POSIX_CALL(close(p2cwrite));
if (c2pread != -1)
POSIX_CALL(close(c2pread));
if (errread != -1)
POSIX_CALL(close(errread));
POSIX_CALL(close(errpipe_read));
There was a problem hiding this comment.
If we decide to skip the close(), I would prefer to also modify _posixsubprocess. But here I only want to use posix_spawn() in more cases, so I prefer to not modify "unrelated" changes :-)
* subprocess.Popen can now also use os.posix_spawn() with pipes if pipe file descriptors are greater than 2. * Fix Popen._posix_spawn(): set _child_created to True. * Add Popen._close_pipe_fds() helper function to factorize the code.
|
@gpshead @giampaolo: would you mind to review this change? It allows to use posix_spawn() in subprocess in much more cases. |
|
If I don't get any feedback before the end of the week, I will just merge my PR. I prefer to merge it early during the 3.8 development cycle, to get more time to fix it if something goes wrong :) |
| os.close(errwrite) | ||
|
|
||
| if devnull_fd is not None: | ||
| os.close(devnull_fd) |
There was a problem hiding this comment.
In case of premature failure on X.Close() or os.close(X) the remaining pipes/fds will remain "open". Perhaps it makes sense to use contextlib.ExitStack.
There was a problem hiding this comment.
Is it a regression caused by my PR? Or a general remark on existing code? My PR just moves code, no?
There was a problem hiding this comment.
Yep, it doesn't look like a regression introduced by you (was already there).
There was a problem hiding this comment.
At least, I factorized the code so it should help to enhance the code ;-) Maybe open an issue if you want to work on that? I don't see an obvious pattern to ensure that all file descriptors are properly closed.
There was a problem hiding this comment.
Yeah, makes sense. I bet there are many other places which can benefit from ExitStack in a similar manner.
|
When you're done making the requested changes, leave the comment: |
There was a problem hiding this comment.
@giampaolo: So what do you think of the overall change? :-)
| os.close(errwrite) | ||
|
|
||
| if devnull_fd is not None: | ||
| os.close(devnull_fd) |
There was a problem hiding this comment.
At least, I factorized the code so it should help to enhance the code ;-) Maybe open an issue if you want to work on that? I don't see an obvious pattern to ensure that all file descriptors are properly closed.
|
@giampaolo: "LGTM" Yay! Thanks for the approval! I merged my PR. |
|
@giampaolo: As I wrote, please open a new issue if you would like to enhance the new _close_pipe_fds() helper method. |
Close pipes/fds in subprocess by using ExitStack. "In case of premature failure on X.Close() or os.close(X) the remaining pipes/fds will remain "open". Perhaps it makes sense to use contextlib.ExitStack." - Rationale: #11575 (comment)
pipe file descriptors are greater than 2.
https://bugs.python.org/issue35537