Patchwork sshpeer: store subprocess so it cleans up correctly

login
register
mail settings
Submitter Durham Goode
Date March 9, 2013, 1:21 a.m.
Message ID <f22f59d4265e10959313.1362792083@dev350.prn1.facebook.com>
Download mbox | patch
Permalink /patch/1093/
State Superseded
Commit 9baf4330d88f81475cadcfd2041fc100f6cf201f
Headers show

Comments

Durham Goode - March 9, 2013, 1:21 a.m.
# HG changeset patch
# User Durham Goode <durham@fb.com>
# Date 1362790776 28800
#      Fri Mar 08 16:59:36 2013 -0800
# Node ID f22f59d4265e10959313f801e7b0d642d154a81f
# Parent  2b1729b20820c0eeb0857bb224d009db698faeef
sshpeer: store subprocess so it cleans up correctly

When running 'hg pull --rebase', I was seeing this exception over 100% of the
time as the python process was closing down:

Exception TypeError: TypeError("'NoneType' object is not callable",) in
<bound method Popen.__del__ of <subprocess.Popen object at 0x937c10>> ignored

By storing the subprocess on the sshpeer, the subprocess seems to clean up
correctly, and I no longer see the exception. I have no idea why this actually
works, but I get a 0% repro if I store the subprocess in self.subprocess,
and a 100% repro if I store None in self.subprocess.

Possibly related to issue 2240.
Durham Goode - March 9, 2013, 1:28 a.m.
On 3/8/13 5:21 PM, "Durham Goode" <durham@fb.com> wrote:

># HG changeset patch
># User Durham Goode <durham@fb.com>
># Date 1362790776 28800
>#      Fri Mar 08 16:59:36 2013 -0800
># Node ID f22f59d4265e10959313f801e7b0d642d154a81f
># Parent  2b1729b20820c0eeb0857bb224d009db698faeef
>sshpeer: store subprocess so it cleans up correctly
>

Ignore that.  My hubris got the best of me.  Seems utils.popen was already
defined and the path I tested just didn't hit the old one.  The other
tests did.  I will resend with passing tests next time :(

Patch

diff --git a/mercurial/sshpeer.py b/mercurial/sshpeer.py
--- a/mercurial/sshpeer.py
+++ b/mercurial/sshpeer.py
@@ -70,7 +70,14 @@ 
                 (_serverquote(remotecmd), _serverquote(self.path))))
         ui.note(_('running %s\n') % cmd)
         cmd = util.quotecommand(cmd)
-        self.pipeo, self.pipei, self.pipee = util.popen3(cmd)
+
+        # while self.subprocess isn't used, having it allows the subprocess to
+        # to clean up correctly later
+        self.subprocess = util.popen(cmd)
+        # in and out are reversed, because it's always been that way
+        self.pipeo = self.subprocess.stdin
+        self.pipei = self.subprocess.stdout
+        self.pipee = self.subprocess.stderr
 
         # skip any noise generated by remote shell
         self._callstream("hello")
diff --git a/mercurial/util.py b/mercurial/util.py
--- a/mercurial/util.py
+++ b/mercurial/util.py
@@ -117,6 +117,14 @@ 
 import subprocess
 closefds = os.name == 'posix'
 
+def popen(cmd, env=None, newlines=False):
+    return subprocess.Popen(cmd, shell=True, bufsize=-1,
+                            close_fds=closefds,
+                            stdin=subprocess.PIPE, stdout=subprocess.PIPE,
+                            stderr=subprocess.PIPE,
+                            universal_newlines=newlines,
+                            env=env)
+
 def popen2(cmd, env=None, newlines=False):
     # Setting bufsize to -1 lets the system decide the buffer size.
     # The default for bufsize is 0, meaning unbuffered. This leads to
@@ -129,12 +137,7 @@ 
     return p.stdin, p.stdout
 
 def popen3(cmd, env=None, newlines=False):
-    p = subprocess.Popen(cmd, shell=True, bufsize=-1,
-                         close_fds=closefds,
-                         stdin=subprocess.PIPE, stdout=subprocess.PIPE,
-                         stderr=subprocess.PIPE,
-                         universal_newlines=newlines,
-                         env=env)
+    p = popen(cmd, env, newlines)
     return p.stdin, p.stdout, p.stderr
 
 def version():