cpython
cpython copied to clipboard
python
gh-87474: Fix file descriptor leaks in subprocess.Popen
Rationale in #87474. Didn't PR until now because there were no further comments on that issue, and the immediate problem I was seeing was resolved.
- Issue: gh-87474
Rather than logic that individually keeps track of which things need closing and duplicates decision logic as to which were opened within the exception handler, a cleaner less difficult to mess up tactic could be to use a fd closing context manager that allows registration of fds, and closes them all when the context manager __exit__ is called with an exception rather than a clean exit.
A method something like this perhaps?
@contextlib.contextmanager
def _on_error_fd_closer(self):
to_close = []
try:
yield o_close
except:
if hasattr(self, '_devnull):
fds_to_close.append(self._devnull)
del self._devnull
for fd in set(to_close):
if windows_handle_close := getattr(fd, 'Close'):
windows_handle_close()
else:
os.close(fd) # wrap this in its own try: ... except: pass for good measure?
raise
with self._on_error_fd_closer() as fds:
x, y = os.pipe()
fds.extend((x, y))
...
# stuff happens
...
If an exception occurs, the file descriptors registered by adding them to the context's list of things to cleanup will be closed.
This way it becomes an RAII pattern, the code maintenance mistake that could be made is forgetting to add a fd or handle to the list of things to close. Which should be much more obvious in the code as that would usually happen immediately after its creation instead of duplicate logic in an error handler not within visual range.
@gpshead That's definitely much cleaner. I've incorporated a slightly modified version of that context manager (written to more closely mirror the cleanup code at the end of __init__).
@gpshead any other changes you wanted to see/make to this PR before merging?
@gpshead it's been a while and I think I've addressed all your concerns; any way to get this merged soon?
Thanks @cptpcrd for the PR, and @gpshead for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11. 🐍🍒⛏🤖
