classification
Title: race condition in SocketServer.py ForkingMixIn collect_children
Type: behavior Stage: resolved
Components: Library (Lib) Versions: Python 3.4, Python 3.3, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: haypo, idsvandermolen, neologix, pitrou, python-dev
Priority: normal Keywords: needs review, patch

Created on 2014-05-13 08:49 by idsvandermolen, last changed 2014-06-21 09:10 by neologix. This issue is now closed.

Files
File name Uploaded Description Edit
socketserver_reap.diff neologix, 2014-06-07 12:05 review
Messages (6)
msg218414 - (view) Author: Ids van der Molen (idsvandermolen) Date: 2014-05-13 08:49
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))
msg219930 - (view) Author: Charles-François Natali (neologix) * (Python committer) Date: 2014-06-07 12:05
Here's a patch fixing both issues.
msg221122 - (view) Author: Roundup Robot (python-dev) Date: 2014-06-20 21:03
New changeset aa5e3f7a5501 by Charles-François Natali in branch '2.7':
Issue #21491: SocketServer: Fix a race condition in child processes reaping.
http://hg.python.org/cpython/rev/aa5e3f7a5501
msg221124 - (view) Author: Roundup Robot (python-dev) Date: 2014-06-20 21:42
New changeset 2a7375bd09f9 by Charles-François Natali in branch '3.4':
Issue #21491: socketserver: Fix a race condition in child processes reaping.
http://hg.python.org/cpython/rev/2a7375bd09f9
msg221126 - (view) Author: Roundup Robot (python-dev) Date: 2014-06-20 21:51
New changeset ae0b572ced20 by Charles-François Natali in branch 'default':
Issue #21491: socketserver: Fix a race condition in child processes reaping.
http://hg.python.org/cpython/rev/ae0b572ced20
msg221163 - (view) Author: Charles-François Natali (neologix) * (Python committer) Date: 2014-06-21 09:10
Committed, thanks!
History
Date User Action Args
2014-06-21 09:10:17neologixsetstatus: open -> closed
versions: + Python 3.3, Python 3.4
type: resource usage -> behavior
messages: + msg221163

resolution: fixed
stage: patch review -> resolved
2014-06-20 21:51:21python-devsetmessages: + msg221126
2014-06-20 21:42:38python-devsetmessages: + msg221124
2014-06-20 21:03:53python-devsetnosy: + python-dev
messages: + msg221122
2014-06-07 12:05:44neologixsetfiles: + socketserver_reap.diff

nosy: + pitrou, haypo
messages: + msg219930

keywords: + patch, needs review
stage: patch review
2014-05-13 08:59:10pitrousetnosy: + neologix
2014-05-13 08:49:14idsvandermolencreate