Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

race condition in SocketServer.py ForkingMixIn collect_children #65690

Closed
idsvandermolen mannequin opened this issue May 13, 2014 · 6 comments
Closed

race condition in SocketServer.py ForkingMixIn collect_children #65690

idsvandermolen mannequin opened this issue May 13, 2014 · 6 comments
Labels
stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@idsvandermolen
Copy link
Mannequin

idsvandermolen mannequin commented May 13, 2014

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
    Copy link
    Mannequin Author

    idsvandermolen mannequin commented May 13, 2014

    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))

    @idsvandermolen idsvandermolen mannequin added performance Performance or resource usage stdlib Python modules in the Lib dir labels May 13, 2014
    @neologix
    Copy link
    Mannequin

    neologix mannequin commented Jun 7, 2014

    Here's a patch fixing both issues.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jun 20, 2014

    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
    Copy link
    Mannequin

    python-dev mannequin commented Jun 20, 2014

    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
    Copy link
    Mannequin

    python-dev mannequin commented Jun 20, 2014

    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
    Copy link
    Mannequin

    neologix mannequin commented Jun 21, 2014

    Committed, thanks!

    @neologix neologix mannequin closed this as completed Jun 21, 2014
    @neologix neologix mannequin added type-bug An unexpected behavior, bug, or error and removed performance Performance or resource usage labels Jun 21, 2014
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    0 participants