From af4db37f2a432c809b19452648f66742a7ccc9e1 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Charles-Fran=C3=A7ois=20Natali?= <cf.natali@gmail.com>
Date: Fri, 20 Jun 2014 22:49:26 +0100
Subject: [PATCH] Issue #21491: socketserver: Fix a race condition in child
 processes reaping.

---
 Lib/socketserver.py | 58 ++++++++++++++++++++++++---------------------
 Misc/NEWS           |  2 ++
 2 files changed, 33 insertions(+), 27 deletions(-)

diff --git a/Lib/socketserver.py b/Lib/socketserver.py
index 7c85fbc9713..b585640b53e 100644
--- a/Lib/socketserver.py
+++ b/Lib/socketserver.py
@@ -539,35 +539,39 @@ class ForkingMixIn:
 
     def collect_children(self):
         """Internal routine to wait for children that have exited."""
-        if self.active_children is None: return
-        while len(self.active_children) >= self.max_children:
-            # XXX: This will wait for any child process, not just ones
-            # spawned by this library. This could confuse other
-            # libraries that expect to be able to wait for their own
-            # children.
-            try:
-                pid, status = os.waitpid(0, 0)
-            except OSError:
-                pid = None
-            if pid not in self.active_children: continue
-            self.active_children.remove(pid)
+        if self.active_children is None:
+            return
 
-        # XXX: This loop runs more system calls than it ought
-        # to. There should be a way to put the active_children into a
-        # process group and then use os.waitpid(-pgid) to wait for any
-        # of that set, but I couldn't find a way to allocate pgids
-        # that couldn't collide.
-        for child in self.active_children:
+        # If we're above the max number of children, wait and reap them until
+        # we go back below threshold. Note that we use waitpid(-1) below to be
+        # able to collect children in size(<defunct children>) syscalls instead
+        # of size(<children>): the downside is that this might reap children
+        # which we didn't spawn, which is why we only resort to this when we're
+        # above max_children.
+        while len(self.active_children) >= self.max_children:
             try:
-                pid, status = os.waitpid(child, os.WNOHANG)
+                pid, _ = os.waitpid(-1, 0)
+                self.active_children.discard(pid)
+            except InterruptedError:
+                pass
+            except ChildProcessError:
+                # we don't have any children, we're done
+                self.active_children.clear()
             except OSError:
-                pid = None
-            if not pid: continue
+                break
+
+        # Now reap all defunct children.
+        for pid in self.active_children.copy():
             try:
-                self.active_children.remove(pid)
-            except ValueError as e:
-                raise ValueError('%s. x=%d and list=%r' % (e.message, pid,
-                                                           self.active_children))
+                pid, _ = os.waitpid(pid, os.WNOHANG)
+                # if the child hasn't exited yet, pid will be 0 and ignored by
+                # discard() below
+                self.active_children.discard(pid)
+            except ChildProcessError:
+                # someone else reaped it
+                self.active_children.discard(pid)
+            except OSError:
+                pass
 
     def handle_timeout(self):
         """Wait for zombies after self.timeout seconds of inactivity.
@@ -589,8 +593,8 @@ class ForkingMixIn:
         if pid:
             # Parent process
             if self.active_children is None:
-                self.active_children = []
-            self.active_children.append(pid)
+                self.active_children = set()
+            self.active_children.add(pid)
             self.close_request(request)
             return
         else:
diff --git a/Misc/NEWS b/Misc/NEWS
index f6491340d33..81bf254f9c5 100644
--- a/Misc/NEWS
+++ b/Misc/NEWS
@@ -103,6 +103,8 @@ Core and Builtins
 Library
 -------
 
+- Issue #21491: socketserver: Fix a race condition in child processes reaping.
+
 - Issue #21719: Added the ``st_file_attributes`` field to os.stat_result on
   Windows.