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

utils/network.py: fd not closed properly #271

Closed
supertylerc opened this Issue Apr 18, 2016 · 2 comments

Comments

Projects
None yet
2 participants
@supertylerc
Contributor

supertylerc commented Apr 18, 2016

Minor bug introduced by #267. These lines redirect output to os.devnull, but the fd isn't properly closed. Here's an example of the issue on OS X.

Example Python Script

#!/usr/bin/env python2
import subprocess
x = open('/dev/null', 'w')
subprocess.call(['ls'], stdout=x, stderr=x, close_fds=True)
input('go check the fd!')

Verifying the Bug

$ python harbl.py

# In a separate terminal:
$ ps -p $(lsof /dev/null | grep python | awk '{print $2}')
  PID TTY           TIME CMD
35608 ttys005    0:00.02 python harbl.py

Fixing the Bug

To fix the issue, either try/finally needs to be used or (preferably) a context manager, similar to below:

#!/usr/bin/env python2
import subprocess
with open('/dev/null', 'w') as f:
    subprocess.call(['ls'], stdout=f, stderr=f, close_fds=True)
input('go check the fd!')

And validating:

$ python harbl.py
check the fd!

# In a separate terminal:
$ lsof /dev/null | grep python
$

I could submit the PR to correct this.

@jathanism

This comment has been minimized.

Member

jathanism commented Apr 18, 2016

Well that's no good. The context manager is certainly a better approach. Good thing you're catching this before we released it. ;-)

@supertylerc

This comment has been minimized.

Contributor

supertylerc commented Apr 18, 2016

Thanks. I'll submit the PR in an hour or two. Out and about for a bit! :)

supertylerc added a commit to supertylerc/trigger that referenced this issue Apr 18, 2016

Cleanly close /dev/null fd in `utils.network.ping`
Cleanly closes a file descriptor in `trigger.utils.network.ping`.
Although exiting a function would forcefully close the fd, it's better
to close it cleanly and explicitly.

Fixes trigger#271

jathanism added a commit that referenced this issue Apr 18, 2016

Cleanly close /dev/null fd in `utils.network.ping` (#272)
Cleanly closes a file descriptor in `trigger.utils.network.ping`.
Although exiting a function would forcefully close the fd, it's better
to close it cleanly and explicitly.

Fixes #271
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment