From: Gregory P. Smith <greg@krypto.org>
Date: Tue, 5 Sep 2017 18:20:02 +0000 (-0700)
Subject: bpo-27448: Work around a gc.disable race condition in subprocess. (#1932)
X-Git-Tag: v2.7.15rc1~224
X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=5e8e371364ee58dadb9a4e4e51c7e9cf6bedbfae;p=python

bpo-27448: Work around a gc.disable race condition in subprocess. (#1932)

* bpo-27448: Work around a gc.disable race condition in subprocess.

This works around a gc.isenabled/gc.disable race condition in the 2.7
subprocess module by using a lock for the critical section.  It'll
prevent multiple simultaneous subprocess launches from winding up with
gc remaining disabled but it can't fix the ultimate problem: gc enable
and disable is a global setting and a hack.

Users are *strongly encouraged* to use subprocess32 from PyPI instead
of the 2.7 standard library subprocess module.  Mixing threads with
subprocess is a recipie for disaster otherwise even with "fixes" to
ameliorate common issues like this.

* Add a blurb!
---

diff --git a/Lib/subprocess.py b/Lib/subprocess.py
index 4b41f5ec5c..1f2da0ffbe 100644
--- a/Lib/subprocess.py
+++ b/Lib/subprocess.py
@@ -71,6 +71,10 @@ if mswindows:
 else:
     import select
     _has_poll = hasattr(select, 'poll')
+    try:
+        import threading
+    except ImportError:
+        threading = None
     import fcntl
     import pickle
 
@@ -878,6 +882,21 @@ class Popen(object):
                         pass
 
 
+        # Used as a bandaid workaround for https://bugs.python.org/issue27448
+        # to prevent multiple simultaneous subprocess launches from interfering
+        # with one another and leaving gc disabled.
+        if threading:
+            _disabling_gc_lock = threading.Lock()
+        else:
+            class _noop_context_manager(object):
+                # A dummy context manager that does nothing for the rare
+                # user of a --without-threads build.
+                def __enter__(self): pass
+                def __exit__(self, *args): pass
+
+            _disabling_gc_lock = _noop_context_manager()
+
+
         def _execute_child(self, args, executable, preexec_fn, close_fds,
                            cwd, env, universal_newlines,
                            startupinfo, creationflags, shell, to_close,
@@ -909,10 +928,12 @@ class Popen(object):
             errpipe_read, errpipe_write = self.pipe_cloexec()
             try:
                 try:
-                    gc_was_enabled = gc.isenabled()
-                    # Disable gc to avoid bug where gc -> file_dealloc ->
-                    # write to stderr -> hang.  http://bugs.python.org/issue1336
-                    gc.disable()
+                    with self._disabling_gc_lock:
+                        gc_was_enabled = gc.isenabled()
+                        # Disable gc to avoid bug where gc -> file_dealloc ->
+                        # write to stderr -> hang.
+                        # https://bugs.python.org/issue1336
+                        gc.disable()
                     try:
                         self.pid = os.fork()
                     except:
@@ -986,9 +1007,10 @@ class Popen(object):
                             exc_value.child_traceback = ''.join(exc_lines)
                             os.write(errpipe_write, pickle.dumps(exc_value))
 
-                        # This exitcode won't be reported to applications, so it
-                        # really doesn't matter what we return.
-                        os._exit(255)
+                        finally:
+                            # This exitcode won't be reported to applications, so it
+                            # really doesn't matter what we return.
+                            os._exit(255)
 
                     # Parent
                     if gc_was_enabled:
diff --git a/Misc/NEWS.d/next/Library/2017-09-05-10-55-50.bpo-27448.QdAqzZ.rst b/Misc/NEWS.d/next/Library/2017-09-05-10-55-50.bpo-27448.QdAqzZ.rst
new file mode 100644
index 0000000000..9e269858d4
--- /dev/null
+++ b/Misc/NEWS.d/next/Library/2017-09-05-10-55-50.bpo-27448.QdAqzZ.rst
@@ -0,0 +1,5 @@
+Work around a `gc.disable()` race condition in the `subprocess` module that
+could leave garbage collection disabled when multiple threads are spawning
+subprocesses at once.  Users are *strongly encouraged* to use the
+`subprocess32` module from PyPI on Python 2.7 instead, it is much more
+reliable.