Bug #1183780: Add Popen objects to _active only in __del__.

Cleanup terminated processes as well.
Add cmd attribute to Popen4.

2 files changed, 23 insertions, 7 deletions

diff --git a/Lib/popen2.py b/Lib/popen2.py
index a4eda39..1529f7e 100644
--- a/Lib/popen2.py
+++ b/Lib/popen2.py
@@ -20,7 +20,13 @@ _active = []
 
 def _cleanup():
     for inst in _active[:]:
-        inst.poll()
+        if inst.poll(_deadstate=sys.maxint) >= 0:
+            try:
+                _active.remove(inst)
+            except ValueError:
+                # This can happen if two threads create a new Popen instance.
+                # It's harmless that it was already removed, so ignore.
+                pass
 
 class Popen3:
     """Class representing a child process.  Normally instances are created
@@ -61,7 +67,13 @@ class Popen3:
             self.childerr = os.fdopen(errout, 'r', bufsize)
         else:
             self.childerr = None
-        _active.append(self)
+
+    def __del__(self):
+        # In case the child hasn't been waited on, check if it's done.
+        self.poll(_deadstate=sys.maxint)
+        if self.sts < 0:
+            # Child is still running, keep us alive until we can wait on it.
+            _active.append(self)
 
     def _run_child(self, cmd):
         if isinstance(cmd, basestring):
@@ -76,7 +88,7 @@ class Popen3:
         finally:
             os._exit(1)
 
-    def poll(self):
+    def poll(self, _deadstate=None):
         """Return the exit status of the child process if it has finished,
         or -1 if it hasn't finished yet."""
         if self.sts < 0:
@@ -84,9 +96,9 @@ class Popen3:
                 pid, sts = os.waitpid(self.pid, os.WNOHANG)
                 if pid == self.pid:
                     self.sts = sts
-                    _active.remove(self)
             except os.error:
-                pass
+                if _deadstate is not None:
+                    self.sts = _deadstate
         return self.sts
 
     def wait(self):
@@ -95,7 +107,6 @@ class Popen3:
             pid, sts = os.waitpid(self.pid, 0)
             if pid == self.pid:
                 self.sts = sts
-                _active.remove(self)
         return self.sts
 
 
@@ -104,6 +115,7 @@ class Popen4(Popen3):
 
     def __init__(self, cmd, bufsize=-1):
         _cleanup()
+        self.cmd = cmd
         p2cread, p2cwrite = os.pipe()
         c2pread, c2pwrite = os.pipe()
         self.pid = os.fork()
@@ -117,7 +129,6 @@ class Popen4(Popen3):
         self.tochild = os.fdopen(p2cwrite, 'w', bufsize)
         os.close(c2pwrite)
         self.fromchild = os.fdopen(c2pread, 'r', bufsize)
-        _active.append(self)
 
 
 if sys.platform[:3] == "win" or sys.platform == "os2emx":
@@ -220,6 +231,7 @@ def _test():
         raise ValueError("unexpected %r on stderr" % (got,))
     for inst in _active[:]:
         inst.wait()
+    _cleanup()
     if _active:
         raise ValueError("_active not empty")
     print "All OK"
diff --git a/Lib/test/test_popen2.py b/Lib/test/test_popen2.py
index 18142ec..4db3cd1 100644
--- a/Lib/test/test_popen2.py
+++ b/Lib/test/test_popen2.py
@@ -35,6 +35,9 @@ def _test():
     # same test as popen2._test(), but using the os.popen*() API
     print "Testing os module:"
     import popen2
+    # When the test runs, there shouldn't be any open pipes
+    popen2._cleanup()
+    assert not popen2._active, "Active pipes when test starts " + repr([c.cmd for c in popen2._active])
     cmd  = "cat"
     teststr = "ab cd\n"
     if os.name == "nt":
@@ -65,6 +68,7 @@ def _test():
         raise ValueError("unexpected %r on stderr" % (got,))
     for inst in popen2._active[:]:
         inst.wait()
+    popen2._cleanup()
     if popen2._active:
         raise ValueError("_active not empty")
     print "All OK"