subprocess.Popen.__del__ referenced global objects, which is a no-no thanks to

interpreter shutdown semantics. Same issue goes for the methods that __del__
called. Now all the methods capture the global objects it needs as default
values to private parameters (could have stuck them on the class object itself,
but since the objects have nothing directly to do with the class that seemed
wrong).

There is no test as making one that works is hard. This patch was
verified against a consistently failing test in Mercurial's test suite, though,
so it has been tested in some regard.

Closes issue #5099. Thanks to Mary Stern for the bug report and Gabriel
Genellina for writing another patch for the same issue and attempting to write
a test.

diff --git a/Lib/subprocess.py b/Lib/subprocess.py
index 8c687c9639afd4619df03738cc7db370d838181c..4a894fe58768a689871549729faedb1279cb23d4 100644 (file)
--- a/Lib/subprocess.py
+++ b/Lib/subprocess.py
@@ -413,7 +413,6 @@ class CalledProcessError(Exception):
 
 
 if mswindows:
-    from _subprocess import CREATE_NEW_CONSOLE, CREATE_NEW_PROCESS_GROUP
     import threading
     import msvcrt
     import _subprocess
@@ -442,6 +441,7 @@ __all__ = ["Popen", "PIPE", "STDOUT", "call", "check_call",
            "check_output", "CalledProcessError"]
 
 if mswindows:
+    from _subprocess import CREATE_NEW_CONSOLE, CREATE_NEW_PROCESS_GROUP
     __all__.extend(["CREATE_NEW_CONSOLE", "CREATE_NEW_PROCESS_GROUP"])
 try:
     MAXFD = os.sysconf("SC_OPEN_MAX")
@@ -699,12 +699,12 @@ class Popen(object):
         return data
 
 
-    def __del__(self, sys=sys):
+    def __del__(self, _maxint=sys.maxint, _active=_active):
         if not self._child_created:
             # We didn't get to successfully create a child process.
             return
         # In case the child hasn't been waited on, check if it's done.
-        self._internal_poll(_deadstate=sys.maxint)
+        self._internal_poll(_deadstate=_maxint)
         if self.returncode is None and _active is not None:
             # Child is still running, keep us alive until we can wait on it.
             _active.append(self)
@@ -907,13 +907,20 @@ class Popen(object):
                 errwrite.Close()
 
 
-        def _internal_poll(self, _deadstate=None):
+        def _internal_poll(self, _deadstate=None,
+                _WaitForSingleObject=WaitForSingleObject,
+                _WAIT_OBJECT_0=WAIT_OBJECT_0,
+                _GetExitCodeProcess=GetExitCodeProcess):
             """Check if child process has terminated.  Returns returncode
-            attribute."""
+            attribute.
+
+            This method is called by __del__, so it can only refer to objects
+            in its local scope.
+
+            """
             if self.returncode is None:
-                if(_subprocess.WaitForSingleObject(self._handle, 0) ==
-                   _subprocess.WAIT_OBJECT_0):
-                    self.returncode = _subprocess.GetExitCodeProcess(self._handle)
+                if _WaitForSingleObject(self._handle, 0) == _WAIT_OBJECT_0:
+                    self.returncode = _GetExitCodeProcess(self._handle)
             return self.returncode
 
 
@@ -1194,25 +1201,35 @@ class Popen(object):
                 raise child_exception
 
 
-        def _handle_exitstatus(self, sts):
-            if os.WIFSIGNALED(sts):
-                self.returncode = -os.WTERMSIG(sts)
-            elif os.WIFEXITED(sts):
-                self.returncode = os.WEXITSTATUS(sts)
+        def _handle_exitstatus(self, sts, _WIFSIGNALED=os.WIFSIGNALED,
+                _WTERMSIG=os.WTERMSIG, _WIFEXITED=os.WIFEXITED,
+                _WEXITSTATUS=os.WEXITSTATUS):
+            # This method is called (indirectly) by __del__, so it cannot
+            # refer to anything outside of its local scope."""
+            if _WIFSIGNALED(sts):
+                self.returncode = -_WTERMSIG(sts)
+            elif _WIFEXITED(sts):
+                self.returncode = _WEXITSTATUS(sts)
             else:
                 # Should never happen
                 raise RuntimeError("Unknown child exit status!")
 
 
-        def _internal_poll(self, _deadstate=None):
+        def _internal_poll(self, _deadstate=None, _waitpid=os.waitpid,
+                _WNOHANG=os.WNOHANG, _os_error=os.error):
             """Check if child process has terminated.  Returns returncode
-            attribute."""
+            attribute.
+
+            This method is called by __del__, so it cannot reference anything
+            outside of the local scope (nor can any methods it calls).
+
+            """
             if self.returncode is None:
                 try:
-                    pid, sts = os.waitpid(self.pid, os.WNOHANG)
+                    pid, sts = _waitpid(self.pid, _WNOHANG)
                     if pid == self.pid:
                         self._handle_exitstatus(sts)
-                except os.error:
+                except _os_error:
                     if _deadstate is not None:
                         self.returncode = _deadstate
             return self.returncode

diff --git a/Misc/NEWS b/Misc/NEWS
index b625e7b6bfc7fd92f8c7514ea22b7aaaefd2bdd0..fe6ebc1c217015db000ccd3e68bf575c5c81c85e 100644 (file)
--- a/Misc/NEWS
+++ b/Misc/NEWS
@@ -15,6 +15,10 @@ Core and Builtins
 Library
 -------
 
+- Issue #5099: subprocess.Popen's __del__ method (and the methods it calls)
+  referenced global objects, causing errors to pop up during interpreter
+  shutdown.
+
 Extension Modules
 -----------------