gh-87474: Fix file descriptor leaks in subprocess.Popen

[cptpcrd]

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.

Fixes #87474.

• Issue: subprocess.Popen leaks file descriptors opened for DEVNULL or PIPE stdin/stdout/stderr arguments #87474

[gpshead]

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.

[cptpcrd]

@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__).

[cptpcrd]

@gpshead any other changes you wanted to see/make to this PR before merging?

[cptpcrd]

@gpshead pinging again, I don't want this to fall through the cracks.

[cptpcrd]

@gpshead it's been a while and I think I've addressed all your concerns; any way to get this merged soon?

[miss-islington]

Thanks @cptpcrd for the PR, and @gpshead for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11.
🐍🍒⛏🤖

[bedevere-bot]

GH-104563 is a backport of this pull request to the 3.11 branch.