socketserver.ForkingMixIn leaves zombies, also fails to reap all zombies in one pass

[jwarkentin]

BPO	11109
Nosy	@gpshead, @orsenthil, @vstinner, @giampaolo, @iritkatriel
Files	collect_children.patch: Patch to collect children in serve_forever() instead of process_request() loop_actions.patch: Defines a _loop_actions() method that can be overriden by a subclass. Implements collect_children() in ForkingMixIn._loop_actions(). loop_actions.patch: Same as previous patch, but renames _loop_actions() to loop_actions() and adds it to the list of overridable methods.

^{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 = 'https://github.com/orsenthil'
closed_at = <Date 2021-11-29.23:39:08.897>
created_at = <Date 2011-02-03.22:01:13.247>
labels = ['type-bug', 'library']
title = 'socketserver.ForkingMixIn leaves zombies, also fails to reap all zombies in one pass'
updated_at = <Date 2021-11-29.23:39:08.896>
user = 'https://bugs.python.org/jwarkentin'

bugs.python.org fields:

activity = <Date 2021-11-29.23:39:08.896>
actor = 'iritkatriel'
assignee = 'orsenthil'
closed = True
closed_date = <Date 2021-11-29.23:39:08.897>
closer = 'iritkatriel'
components = ['Library (Lib)']
creation = <Date 2011-02-03.22:01:13.247>
creator = 'jwarkentin'
dependencies = []
files = ['20665', '20693', '20695']
hgrepos = []
issue_num = 11109
keywords = ['patch']
message_count = 20.0
messages = ['127825', '127972', '128026', '128033', '128036', '128037', '128039', '128045', '132441', '132519', '136392', '136760', '136866', '136867', '136869', '136894', '136930', '136933', '171769', '407336']
nosy_count = 7.0
nosy_names = ['gregory.p.smith', 'orsenthil', 'vstinner', 'giampaolo.rodola', 'jwarkentin', 'python-dev', 'iritkatriel']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue11109'
versions = ['Python 3.3']

[jwarkentin]

This is the same issue as was reported here: http://bugs.python.org/issue1954. It is still a problem in Python 3.1. I'm writing a server that will be receiving a massive number of requests and I'd like to eliminate the zombie problem. Once I figured out what was going on, I tried adding a call to collect_children() in the serve_forever() loop. It worked very well. I've included a patch of what I did, however, I obviously can't leave this change in my socketserver.py because we will be deploying this on a lot of servers.

Is there any reason not to collect_children() in the serve_forever() loop? It seems like the place to do it to me. The patch will only collect children if there are any, so it doesn't have to call it every time through the loop.

[jwarkentin]

I guess I didn't really explain the issue much. The problem is that if the server receives say, 10 requests at once, and then forks a process for each, after those processes finish they will sit as zombies until process_request() is called again, which calls collect_children(). So that patch simply moves the call to collect_children() into the serve_forever() loop so that it doesn't leave zombies sitting around.

[gpshead]

Rather than depending on the internal details of ForkingMixIn in your BaseServer.serve_forever modification I'd prefer to see that simply call self._cleanup()

Define a do-nothing _periodic_cleanup method in BaseServer. ForkingMixIn should implement its own _periodic_cleanup method that does the active_children test and calls collect_children as appropriate.

[jwarkentin]

Good point. I was just writing up something quick that works. Here's another patch, is this acceptable?

[gpshead]

I believe that is good. I'll commit it after the 3.2 release has been cut (we're in release candidate release blocker only lockdown right now).

Looking at ForkingMixIn.collect_children() there appears to be another buglet: it loops over self.active_children and calls self.active_children.remove(pid). This modification of the list while looping over it will cause it to skip the next item in the list. For every child waited on successfully, it skips checking one of the others.

[jwarkentin]

I noticed that ForkingMixIn also was overriding handle_timeout() trying to cleanup zombies after 300 seconds of inactivity, which is useless on a busy server. I'm replacing the patch with one that also removes handle_timeout().

[jwarkentin]

I hope I did that last patch right. I did a 'diff -u' instead of a 'diff -c'. If you need something different, let me know.

[jwarkentin]

Sorry I keep plaguing this with comments and files, but I got to thinking, anyone should be able to override _loop_actions() and implement what they need in the loop. While it's probably a bad idea in most cases, there may be legitimate needs and it's always good to allow the flexibility. So, I'm adding one more patch that changes the name to loop_actions() and adds it to the list of methods that can be overridden in the BaseServer docstring. If you like it, keep it, if not, just use the last one.

[jwarkentin]

Just following up on this. Now that 3.2 is out, has the patch been committed?

[gpshead]

not yet, thanks for the reminder. if any other committers feel like jumping on this and doing it before I get around to it, feel free.

[jwarkentin]

Don't mean to nag. Just checking to see if anyone has taken it upon themselves to commit this yet since it's been a couple more months :P

[orsenthil]

Justin,

The patch and logic is okay. We can have this is 3.3.

• I find that loop_actions as not appropriate name for the new method. It fails to give a intuitive meaning of what is supposed to do. request_action, request_action_continued or anything else which gives a meaning should be helpful. I agree with your reasoning to provide some flexiblity for the user to override this.

• The patch lacks Documentation and tests should be added to Lib/test/test_socketserver.py. If you can, please append the patch with these, otherwise I shall do it.

A suggestion for better method name is a must! :)

Thanks!

[python-dev]

New changeset e2363b1c4bca by Senthil Kumaran in branch 'default':
Fix closes issue bpo-11109 - socketserver.ForkingMixIn leaves zombies, also fails to reap all zombies in one pass.
http://hg.python.org/cpython/rev/e2363b1c4bca

New changeset 3e3cd0ed82bb by Senthil Kumaran in branch 'default':
News entry for bpo-11109.
http://hg.python.org/cpython/rev/3e3cd0ed82bb

[orsenthil]

The feature is in 3.3. I did not remove the handle_timeout method from the Mixin class, because it might be used in the production by the existing servers. It is not appropriate to remove methods all of sudden without deprecation warnings and also it is not required to remove in this case.

Added the Documentation and News entry too.

[orsenthil]

This is fixed in 3.3 now. Keeping it open for test_socketserver update. After a ForkingServer service call, it should be asserted the collect_children routine is run.

[vstinner]

New changeset 3e3cd0ed82bb by Senthil Kumaran in branch 'default':
News entry for bpo-11109.
http://hg.python.org/cpython/rev/3e3cd0ed82bb

Please try to format NEWS entries using "Issue #xxx: xxx" instead of "Issue #xxx - xxx".

[orsenthil]

Victor - Sure, I understand Issue #xxx: desc must be useful while generation reST docs.

[jwarkentin]

Sorry, I haven't had a chance to look at this in a couple days. I've been very busy with work. I'm not sure exactly how to write the test for this, so I don't know that I'd be much help there.

One last thing, I was just looking through the commits and I noticed in the NEWS update (http://hg.python.org/cpython/rev/3e3cd0ed82bb/) you have my name as Justin Wark. The last name is actually Warkentin, I just didn't have anything showing that. That's my fault, sorry. I just updated my profile to show my full name.

[python-dev]

New changeset 991c24b8969d by R David Murray in branch '3.3':
bpo-11109: clean up docs, add whatsnew entry, and fix Justin's last name.
http://hg.python.org/cpython/rev/991c24b8969d

New changeset 1234300bc056 by R David Murray in branch 'default':
Merge bpo-11109: clean up docs, add whatsnew entry, and fix Justin's last name.
http://hg.python.org/cpython/rev/1234300bc056

[iritkatriel]

I've created bpo-45935 for the missing test and will close this as resolved in version 3.3.