RE: cvs commit: httpd-2.0/server/mpm/winnt child.c mpm_winnt.c mpm_winnt.h

["William A. Rowe, Jr." <wr...@rowe-clan.net>]

At 08:15 AM 7/29/2002, you wrote:
>Seperating out the routines that run only in the child (and putting them in
>child.c) is not a bad thing but this patch is difficult to review for
>several reasons:
>
>1. The commit log did not mention the biggest change. Easy to intuit looking
>at the code, but it should have at least been mentioned in the commit log.

The changes to fold out the make_listeners_noninherited should really have
been entirely precommitted, not part of the split.  I didn't foresee the 
problems
until I was quite a ways through, and unfortunately this box has only one tree.

Bigger problem, though.  This body of this change should have occurred with
the original child.c checkin.  That's my bad for missing the fact that it 
hadn't.


>2. You broke the rule to not make more than one conceptual change to the
>code within a single commit. You moved code (to child.c) and made a function
>change (which pools were being used for what) in one commit. These changes
>should have been made as two seperate commits (or perhaps 3 or 4) to make
>review a bit less onorous.

This patch was supposed to be -only- the change to pass [instead of stash]
the pconf for pchild.  I note that pchild is the direct relative of pconf, 
so in the
Win95 queue code, I presumed that pchild is just as safe for that application.
The *intended* patch for mpm_winnt.c simply changed child_main() to pass
the pconf.

Please read the child.c patch on it's own, and forgive that mpm_winnt changes
were all slated (except for the child_main() pconf arg) for the child.c 
checkin.

> > I am chewing on my tounge now to not be nasty.  Where are you going with
> > this?  Perhaps I missed it but was this change discussed on list?  And the
> > commit log message describing the change is quite poor.

The prior commit message is the one you should be interested in.

mpm_winnt.c is now just about as difficult to follow as the old src/main.c.
Our multiuse of globals makes my head spin.  Globals are also quite
dangerous with pool scopeing, it's hard to tell just when they will expire.

I reached the conclusion to attack this split when I ran into the issue of
factoring out the redundant max_requests_per_child_event ... which is
truly a special case of exit_event.  Such a special case that it should
fall into a single enum value in the same grouping as is_graceful.  But we
are inconsistent about how we toggle the various flags prior to signaling
the exit_event, and then wait until we are in the exit_event to toggle
is_graceful.  This is all quite hard to follow.

The real PITA was that I recognized that the parent doesn't know about
the child giving up the ghost until it's dead.  This is a trivial change; we
simply need to keep watching the exit event handle in the parent, as well,
and make it a manual reset event.  If the child's exit event is signalled,
the parent will know we have a child heading out of existence, and we
can preemptively begin a new child process while the old one cycles down.

> > > wrowe       2002/07/28 22:12:50
> > >
> > >   Modified:    server/mpm/winnt child.c mpm_winnt.c mpm_winnt.h
> > >   Log:
> > >     pconf global factors out nicely.  The one other pconf appears to be
> > >     eqivilant to pchild.

The -huge- problem was that this was not a balanced commit.  My fault
that I didn't notice that mpm_winnt change itself was not within the 
following commit; I've added the following comment to mpm_winnt.c's and 
.h's logs.

   Modified:    .        libhttpd.dsp
                os/win32 ap_regkey.c os.h util_win32.c
   Added:       server/mpm/winnt child.c
   Log:
     Refactor out the child behavior from mpm_winnt.  This is the first
     step in making a legible multiprocess windows mpm, or at least
     structuring the code to always begin a new child as an old one is
     going to die soon, rather than waiting for it's final dying breath.

     The only code that had to be affected [due to the split and general
     structure of the code] was merging the set_listeners_noninherited()
     code directly into the get_listeners_from_parent code, and also into
     the apr socket.c code for winnt.  For the most part the code splits
     rather nicely.