GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | ||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | ||||||
GeezerVison is showing github.com — gh-87474: Fix file descriptor leaks in subprocess.Popen by cptpcrd · Pull Request #96351 · python/cpython Conversation
|
|
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 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. |
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
@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 |
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
|
@gpshead any other changes you wanted to see/make to this PR before merging? |
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
@gpshead pinging again, I don't want this to fall through the cracks. |
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
4345a1a to
1f88111
Compare
Simplifies the logic and should help avoid mistakes in the future. Co-authored-by: Gregory P. Smith <[email protected]>
1f88111 to
0d4efba
Compare
|
@gpshead it's been a while and I think I've addressed all your concerns; any way to get this merged soon? |
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
GH-104563 is a backport of this pull request to the 3.11 branch. |
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
…GH-96351) This fixes several ways file descriptors could be leaked from `subprocess.Popen` constructor during error conditions by opening them later and using a context manager "fds to close" registration scheme to ensure they get closed before returning. --------- (cherry picked from commit 3a4c44b) Co-authored-by: cptpcrd <[email protected]> Co-authored-by: Gregory P. Smith [Google] <[email protected]>
) (#104563) gh-87474: Fix file descriptor leaks in subprocess.Popen (GH-96351) This fixes several ways file descriptors could be leaked from `subprocess.Popen` constructor during error conditions by opening them later and using a context manager "fds to close" registration scheme to ensure they get closed before returning. --------- (cherry picked from commit 3a4c44b) Co-authored-by: cptpcrd <[email protected]> Co-authored-by: Gregory P. Smith [Google] <[email protected]>
* main: (26 commits) pythonGH-101520: Move tracemalloc functionality into core, leaving interface in Modules. (python#104508) typing: Add more tests for TypeVar (python#104571) pythongh-104572: Improve error messages for invalid constructs in PEP 695 contexts (python#104573) typing: Use PEP 695 syntax in typing.py (python#104553) pythongh-102153: Start stripping C0 control and space chars in `urlsplit` (python#102508) pythongh-104469: Update README.txt for _testcapi (pythongh-104529) pythonGH-103092: isolate `_elementtree` (python#104561) pythongh-104050: Add typing to Argument Clinic converters (python#104547) pythonGH-103906: Remove immortal refcounting in the interpreter (pythonGH-103909) pythongh-87474: Fix file descriptor leaks in subprocess.Popen (python#96351) pythonGH-103092: isolate `pyexpat` (python#104506) pythongh-75367: Fix data descriptor detection in inspect.getattr_static (python#104517) pythongh-104050: Add more annotations to `Tools/clinic.py` (python#104544) pythongh-104555: Fix isinstance() and issubclass() for runtime-checkable protocols that use PEP 695 (python#104556) pythongh-103865: add monitoring support to LOAD_SUPER_ATTR (python#103866) CODEOWNERS: Assign new PEP 695 files to myself (python#104551) pythonGH-104510: Fix refleaks in `_io` base types (python#104516) pythongh-104539: Fix indentation error in logging.config.rst (python#104545) pythongh-104050: Don't star-import 'types' in Argument Clinic (python#104543) pythongh-104050: Add basic typing to CConverter in clinic.py (python#104538) ...
Uh oh!
There was an error while loading. Please reload this page.
Assignees
Labels
Projects
Milestone
No milestoneDevelopment
Successfully merging this pull request may close these issues.
subprocess.Popen leaks file descriptors opened for DEVNULL or PIPE stdin/stdout/stderr argumentsUh oh!
There was an error while loading. Please reload this page.
7 participants
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.