[Python-checkins] r61106 - in python/trunk/Lib: SocketServer.py test/test_socketserver.py

[Jeffrey Yasskin]

On Thu, Feb 28, 2008 at 10:46 PM, Neal Norwitz <nnorwitz at gmail.com> wrote:
>
> On Thu, Feb 28, 2008 at 10:03 AM, jeffrey.yasskin
>  <python-checkins at python.org> wrote:
>  > Author: jeffrey.yasskin
>  >  Date: Thu Feb 28 19:03:15 2008
>  >  New Revision: 61106
>  >
>  >  Modified:
>  >    python/trunk/Lib/SocketServer.py
>  >    python/trunk/Lib/test/test_socketserver.py
>  >  Log:
>  >  Prevent SocketServer.ForkingMixIn from waiting on child processes that it
>  >  didn't create, in most cases. When there are max_children handlers running, it
>  >  will still wait for any child process, not just handler processes.
>  >
>  >
>  >  Modified: python/trunk/Lib/SocketServer.py
>  >  ==============================================================================
>  >  --- python/trunk/Lib/SocketServer.py    (original)
>  >  +++ python/trunk/Lib/SocketServer.py    Thu Feb 28 19:03:15 2008
>  >  @@ -440,18 +440,30 @@
>  >
>  >      def collect_children(self):
>  >          """Internal routine to wait for children that have exited."""
>  >  -        while self.active_children:
>  >  -            if len(self.active_children) < self.max_children:
>  >  -                options = os.WNOHANG
>  >  -            else:
>  >  -                # If the maximum number of children are already
>  >  -                # running, block while waiting for a child to exit
>  >  -                options = 0
>  >  +        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, options)
>  >  +                pid, status = os.waitpid(0, options=0)
>  >              except os.error:
>  >                  pid = None
>  >  -            if not pid: break
>  >  +            if pid not in self.active_children: continue
>  >  +            self.active_children.remove(pid)
>  >  +
>  >  +        # 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:
>  >  +            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:
>
>  The handling of the exception when removing from active_children seems
>  backwards to me.  In the first loop where we wait on any child, that
>  can return a pid that we might not be expecting.  ISTM there should be
>  a try/except around that one.  It probably should swallow the error.
>  I suppose a nicer API would keep the pid and status and make it
>  available (even though no one will ever check it).

That's what the "if pid not in self.active_children: continue" is for.
Once that passes, if there's no race, the pid can definitely be
removed with no exception. It's an expected situation, so using the
exception to ignore it doesn't seem appropriate.

>  In the second loop (the for loop), we only wait on children we know
>  should be in the list.  If they aren't in the list, this code has a
>  bug.  I think the try/except was added for additional debugging.
>  Hopefully the problem has been fixed, so the extra debugging should no
>  longer be necessary.

Perhaps the ValueError thrown by list.remove() should include that
information. It'd be reasonable to remove this try/except anyway.

-- 
Namasté,
Jeffrey Yasskin