race condition in SocketServer.py ForkingMixIn collect_children

[idsvandermolen]

BPO	21491
Nosy	@pitrou, @vstinner
Files	socketserver_reap.diff

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2014-06-21.09:10:17.628>
created_at = <Date 2014-05-13.08:49:14.719>
labels = ['type-bug', 'library']
title = 'race condition in SocketServer.py ForkingMixIn collect_children'
updated_at = <Date 2014-06-21.09:10:17.603>
user = 'https://bugs.python.org/idsvandermolen'

bugs.python.org fields:

activity = <Date 2014-06-21.09:10:17.603>
actor = 'neologix'
assignee = 'none'
closed = True
closed_date = <Date 2014-06-21.09:10:17.628>
closer = 'neologix'
components = ['Library (Lib)']
creation = <Date 2014-05-13.08:49:14.719>
creator = 'idsvandermolen'
dependencies = []
files = ['35508']
hgrepos = []
issue_num = 21491
keywords = ['patch', 'needs review']
message_count = 6.0
messages = ['218414', '219930', '221122', '221124', '221126', '221163']
nosy_count = 5.0
nosy_names = ['pitrou', 'vstinner', 'idsvandermolen', 'neologix', 'python-dev']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue21491'
versions = ['Python 2.7', 'Python 3.3', 'Python 3.4']

[idsvandermolen]

collect_children routine in SocketServer.py contains two possible race conditions. First one is in while loop "while len(self.active_children) >= self.max_children:". If status of child is collected outside of Socket server (for example in signal handler or something), then the variable self.active_children will not match actual child processes and the os.waitpid(0, 0) within the while loop will raise os.error, errno=10 (ECHILD) "No Child Processes". self.active_children should be emptied in this case, otherwise you'll end up with an endless loop comsuming 100% CPU (as happened to us).

The second possible race condition is below in the collect_children routine in the "for child in self.active_children" which contains a statement self.active_children.remove(pid) which would modify the iterator. I do not now about python 2.7, but before this would result in "incorrect iteration".
Original code:
for child in self.active_children:
try:
pid, status = os.waitpid(child, os.WNOHANG)
except os.error:
pid = None
if not pid: continue
try:
self.active_children.remove(pid)
except ValueError, e:
raise ValueError('%s. x=%d and list=%r' % (e.message, pid, self.active_children))

Fixed code:
to_remove = []
for child in self.active_children:
try:
pid, status = os.waitpid(child, os.WNOHANG)
except os.error:
pid = None
if not pid: continue
to_remove.append(pid)

        for pid in to_remove:
            try:
                self.active_children.remove(pid)
            except ValueError, e:
                raise ValueError('%s. x=%d and list=%r' % (e.message, pid, self.active_children))

[neologix]

Here's a patch fixing both issues.

[python-dev]

New changeset aa5e3f7a5501 by Charles-François Natali in branch '2.7':
Issue bpo-21491: SocketServer: Fix a race condition in child processes reaping.
http://hg.python.org/cpython/rev/aa5e3f7a5501

[python-dev]

New changeset 2a7375bd09f9 by Charles-François Natali in branch '3.4':
Issue bpo-21491: socketserver: Fix a race condition in child processes reaping.
http://hg.python.org/cpython/rev/2a7375bd09f9

[python-dev]

New changeset ae0b572ced20 by Charles-François Natali in branch 'default':
Issue bpo-21491: socketserver: Fix a race condition in child processes reaping.
http://hg.python.org/cpython/rev/ae0b572ced20

[neologix]

Committed, thanks!