-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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 ResourceWarning on process exit (#2472) #2617
Conversation
@gaborbernat I decided on the simpler solution with the context manager to let the stdlib do the heavy lifting |
if debug: | ||
process.communicate() # on purpose not called to make it a background process | ||
|
||
with Popen(cmd, **kwargs) as process: # noqa: S603 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens here? Wouldn't this make it blocking call?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's the point. You can't keep a subprocess open while your main process exits without getting a ResourceWarning
, afaik. Is the intention to keep this process running until it terminates even if the main process finishes?
If you want to continue the rest of the program execution and only wait for the subprocess to finish at the end you'd need to register the process somewhere central and ensure termination just before the main process exits.
Another option maybe is to use close_fds
param of Popen
, but I'm not sure if that fixes the issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally, I'd like to just ignore the resource warning. There's no need for this process to finish when the main process finishes. It's more of a fire and forget kinda thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're lucky, close_fds
seems to do the trick.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nevermind, it doesn't. I was running with --no-periodic-update
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes sorry, after a bunch of research: the proper way to do this is to wait until your subprocess finishes before your application exits. You don't want a zombie process hanging around. Even solutions using asyncio recommend to wait for async tasks to finish before main process exit, else you'll get a warning.
Unfortunately I don't really see a way to register the process with some central application state in this app, it's just not built for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't want a zombie process hanging around.
That's exactly what I want here though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, the unclosed file warning stems from setting pipe = PIPE
in the non-debug case. It disappears if we set it to pipe=subprocess.DEVNULL
instead.
But then we get a different ResourceWarning
: subprocess is still running
The warning was added in 3.6, here: https://bugs.python.org/issue26741
Pretty much everyone saying keeping a zombie process is bad practice, but here we go.
The followup issue mentions some potential solutions to hide the warning, but detach
didn't work: https://bugs.python.org/issue27068
So I had a look at the CPython source, turns out if we set the returncode manually (since we don't care about it anyway), the warning will not be raised: https://github.com/python/cpython/blob/85e5b1f5b806289744ef9a5a13dabfb23044f713/Lib/subprocess.py#L1127 (from main, just to make it a permalink)
Tests passed locally.
c2099d6
to
bc4fda2
Compare
67cf090
to
fc46f11
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for contributing, make sure you address all the checklists (for details on how see development documentation)
tox -e fix
)docs/changelog
folder