Rev 5110: (vila) Close leaked socket to SSH subprocesses (Max Bowsher) in file:///home/pqm/archives/thelove/bzr/2.2/

[Canonical.com Patch Queue Manager]

At file:///home/pqm/archives/thelove/bzr/2.2/

------------------------------------------------------------
revno: 5110 [merge]
revision-id: pqm at pqm.ubuntu.com-20101111095854-ly7aclc4c0bnv2h2
parent: pqm at pqm.ubuntu.com-20101109030238-4bmvi3xtma3o58dy
parent: maxb at f2s.com-20101110084437-pbyuqo9ki61s08kz
committer: Canonical.com Patch Queue Manager <pqm at pqm.ubuntu.com>
branch nick: 2.2
timestamp: Thu 2010-11-11 09:58:54 +0000
message:
  (vila) Close leaked socket to SSH subprocesses (Max Bowsher)
modified:
  NEWS                           NEWS-20050323055033-4e00b5db738777ff
  bzrlib/transport/ssh.py        ssh.py-20060824042150-0s9787kng6zv1nwq-1
=== modified file 'NEWS'
--- a/NEWS	2010-11-08 06:10:41 +0000
+++ b/NEWS	2010-11-10 08:28:31 +0000
@@ -49,6 +49,9 @@
   installed.
   (Martin [gz], Gary van der Merwe, #632465)
 
+* Close leaked socket to SSH subprocesses, which caused dput sftp uploads
+  to hang.  (Max Bowsher, #659590)
+
 Improvements
 ************
 

=== modified file 'bzrlib/transport/ssh.py'
--- a/bzrlib/transport/ssh.py	2010-09-09 07:31:02 +0000
+++ b/bzrlib/transport/ssh.py	2010-11-10 08:44:37 +0000
@@ -361,13 +361,14 @@
             # This platform doesn't support socketpair(), so just use ordinary
             # pipes instead.
             stdin = stdout = subprocess.PIPE
-            sock = None
+            my_sock, subproc_sock = None, None
         else:
             stdin = stdout = subproc_sock
-            sock = my_sock
         proc = subprocess.Popen(argv, stdin=stdin, stdout=stdout,
                                 **os_specific_subprocess_params())
-        return SSHSubprocessConnection(proc, sock=sock)
+        if subproc_sock is not None:
+            subproc_sock.close()
+        return SSHSubprocessConnection(proc, sock=my_sock)
 
     def connect_sftp(self, username, password, host, port):
         try:
@@ -644,25 +645,24 @@
 import weakref
 _subproc_weakrefs = set()
 
-def _close_ssh_proc(proc):
+def _close_ssh_proc(proc, sock):
     """Carefully close stdin/stdout and reap the SSH process.
 
     If the pipes are already closed and/or the process has already been
     wait()ed on, that's ok, and no error is raised.  The goal is to do our best
     to clean up (whether or not a clean up was already tried).
     """
-    dotted_names = ['stdin.close', 'stdout.close', 'wait']
-    for dotted_name in dotted_names:
-        attrs = dotted_name.split('.')
-        try:
-            obj = proc
-            for attr in attrs:
-                obj = getattr(obj, attr)
-        except AttributeError:
-            # It's ok for proc.stdin or proc.stdout to be None.
-            continue
-        try:
-            obj()
+    funcs = []
+    for closeable in (proc.stdin, proc.stdout, sock):
+        # We expect that either proc (a subprocess.Popen) will have stdin and
+        # stdout streams to close, or that we will have been passed a socket to
+        # close, with the option not in use being None.
+        if closeable is not None:
+            funcs.append(closeable.close)
+    funcs.append(proc.wait)
+    for func in funcs:
+        try:
+            func()
         except OSError:
             # It's ok for the pipe to already be closed, or the process to
             # already be finished.
@@ -707,7 +707,7 @@
         # to avoid leaving processes lingering indefinitely.
         def terminate(ref):
             _subproc_weakrefs.remove(ref)
-            _close_ssh_proc(proc)
+            _close_ssh_proc(proc, sock)
         _subproc_weakrefs.add(weakref.ref(self, terminate))
 
     def send(self, data):
@@ -723,7 +723,7 @@
             return os.read(self.proc.stdout.fileno(), count)
 
     def close(self):
-        _close_ssh_proc(self.proc)
+        _close_ssh_proc(self.proc, self._sock)
 
     def get_sock_or_pipes(self):
         if self._sock is not None: