Skip to content
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

Fix leaking file descriptors when subprocess creation fails #2193

Merged
merged 3 commits into from
Jan 7, 2022
Merged

Fix leaking file descriptors when subprocess creation fails #2193

merged 3 commits into from
Jan 7, 2022

Conversation

tjstum
Copy link
Member

@tjstum tjstum commented Dec 9, 2021

If subprocess spawning fails (i.e. unsuccessful call to exec), the
created trio resources (i.e. the trio ends of the pipes that will be
used to send/receive data to the child) don't get cleaned up until the
next garbage collection run (which will call their __del__).

It would be nicer if this was deterministic, and that resources
created during the run of open_process were either completely
transferred to the Process object (which can take over the
management of them) or cleaned up (on failure).

To implement this, two ExitStacks are used: One will manage the
resources that need cleaned up unconditionally (the child ends of the
pipe); the other will manage the resources that need cleaned up on
failure.

Trio currently supports versions of python that lack
contextlib.AsyncExitStack. While there is a backport for 3.6
available on PyPI, the objects that represent the pipes (_FdHolder and
_HandleHolder) don't really need to have an async close. They are
internal implementation details anyway, so this refactors their
implementation to have the Stream/AsyncResource methods on the actual
Stream objects themselves. This allows us to use the regular ExitStack
and avoid an extra dep

@codecov
Copy link

codecov bot commented Dec 9, 2021

Codecov Report

Merging #2193 (d03fd15) into master (7f44dcf) will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #2193      +/-   ##
==========================================
+ Coverage   99.51%   99.55%   +0.04%     
==========================================
  Files         114      114              
  Lines       14759    14778      +19     
  Branches     2344     2344              
==========================================
+ Hits        14687    14712      +25     
+ Misses         47       44       -3     
+ Partials       25       22       -3     
Impacted Files Coverage Δ
trio/_subprocess.py 96.88% <100.00%> (+0.02%) ⬆️
trio/_subprocess_platform/__init__.py 100.00% <100.00%> (ø)
trio/_unix_pipes.py 100.00% <100.00%> (ø)
trio/_windows_pipes.py 100.00% <100.00%> (ø)
trio/tests/test_subprocess.py 100.00% <100.00%> (ø)
trio/tests/test_ssl.py 99.58% <0.00%> (+0.55%) ⬆️
trio/_highlevel_ssl_helpers.py 100.00% <0.00%> (+11.76%) ⬆️

@tjstum
Copy link
Member Author

tjstum commented Dec 9, 2021

/dev/fd certainly isn't guaranteed by POSIX, but it empirically seems to be common enough that it's fine to use in the tests like this.
There doesn't really seem to be an equivalent for Windows. psutil has a Process.open_files() that seems like it could be this, but on Windows, it really only gives you the paths to the files (and not something like an fd). That would also add an extra dep on psutil for the tests. I didn't see any straightforward way to enumerate all the Handles in a process either; there is a sysinternals program that does it, but it appears closed source, and we probably can't redistribute that binary in the tests (plus you'd have to shell out to it and interpret its output, which does not fill me with confidence).

So this is my best suggestion for testing this behavior. Happy to discuss alternatives!

tjstum added 3 commits January 7, 2022 12:44
If subprocess spawning fails (i.e. unsuccessful call to `exec`), the
created trio resources (i.e. the trio ends of the pipes that will be
used to send/receive data to the child) don't get cleaned up until the
next garbage collection run (which will call their `__del__`).

It would be nicer if this was deterministic, and that resources
created during the run of `open_process` were either completely
transferred to the `Process` object (which can take over the
management of them) or cleaned up (on failure).

To implement this, two ExitStacks are used: One will manage the
resources that need cleaned up unconditionally (the child ends of the
pipe); the other will manage the resources that need cleaned up on
failure.

Trio currently supports versions of python that lack
`contextlib.AsyncExitStack`. While there is a backport for 3.6
available on PyPI, the objects that represent the pipes (_FdHolder and
_HandleHolder) don't really need to have an async close. They are
internal implementation details anyway, so this refactors their
implementation to have the Stream/AsyncResource methods on the actual
Stream objects themselves. This allows us to use the regular ExitStack
and avoid an extra dep
Copy link
Member

@Fuyukai Fuyukai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Realistically I don't see any issue with just using /dev/fds because I don't see any actual possibility for the closing behaviour to change on Windows.

@Fuyukai Fuyukai merged commit 7b0001d into python-trio:master Jan 7, 2022
@tjstum tjstum deleted the noleak branch January 7, 2022 21:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants