Fix bug that leaves fds in select after EOF received #154

Closed
wants to merge 1 commit into
from

Projects

None yet

2 participants

@kevinteg
Contributor
kevinteg commented Apr 2, 2013

This fixes an issue I ran into where ssh-agent pipes were left open after we received EOF. I reported it as a fabric issue here:

fabric/fabric#876

This breaks out of the select loop and closes pipes when select returns an fd that is ready with 0 read (EOF).

@bitprophet
Member

Thanks for looking into this. Will attempt to reproduce the 'too many pipes open' issue so I can verify it on my end; at the very least I will confirm that my test suites don't break with this enabled.

@bitprophet
Member

Testing this via Fabric + ssh agent forwarding as discussed in the Fabric + upstream tickets, e.g.:

from fabric.api import *

def foo():
    for _ in range(10):
        run("git remote show origin") # my homedir is a git repo
    run("sleep 30") # for time to lsof/etc locally

invoked as e.g.:

$ fab foo -H a-server

and then in another terminal doing:

$ ps -ax | grep fabric

(too lazy to use os.getpid I guess :D)

followed by:

$ lsof -nP -p <pid found above> | grep PIPE | wc -l

Sure enough it goes up by 2 every time Fab runs a new git command, and never recedes.

About to try this workflow with the patch applied.

@bitprophet
Member

Beautiful, with patch applied it's typically 0 with occasional bumps to 2 as the command actually runs (GH is a tad slow today :D) Thanks for this! Also verified test suites pass. Will add changelog notice and push to the 1.10 branch, then probably cut a 1.10.1 release with this + another bugfix.

@bitprophet bitprophet added a commit that referenced this pull request Apr 5, 2013
@bitprophet bitprophet Changelog re #154 1d494eb
@bitprophet
Member

Github hasn't noticed I merged this, possibly as I merged into the 1.10 release branch and not master. It is in, and 1.10.1 is out on PyPI now. Thanks again!

@bitprophet bitprophet closed this Apr 5, 2013
@kevinteg
Contributor
kevinteg commented Apr 5, 2013

np, thanks for pushing it so quickly!

@kevinteg kevinteg referenced this pull request in cloudenvy/cloudenvy Apr 5, 2013
Closed

Leaky pipes #101

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