Mailing-List: contact new-httpd-help@apache.org; run by ezmlm
Precedence: bulk
Reply-To: new-httpd@apache.org
Date: Mon, 4 Jun 2001 10:43:33 -0700 (PDT)
From: <rbb@covalent.net>
To: <new-httpd@apache.org>
Subject: Re: New graceful restart mechanism.
In-Reply-To: <m366ecuqjx.fsf@adsl-77-241-65.rdu.bellsouth.net>
Message-ID: <Pine.LNX.4.30.0106041028190.28041-100000@koj.rkbloom.net>
MIME-Version: 1.0
Content-Type: TEXT/PLAIN; charset=US-ASCII


> >                                    This could be improved a bit, by using
> > OOB data to wake up the thread, and we would only check the pod if we
> > received OOB data.  This is an optimization however, and I am not sure how
> > that would work in real-life.
>
> Err, I'm not sure either :)  On which connection would you send OOB
> data?  How would we detect that there was OOB data?  If select(),  I
> thought we were removing the need for select() (think
> "SINGLE_LISTEN_UNSERIALIZED_ACCEPT")...   If SIGURG, I thought we were
> avoiding signal-based processing (not sure of SIGURG portability
> anyway)...  I guess I'm missing something real basic here...

I was thinking of doing the connect, and actually sending OOB data over
the wire.  I seem to recall some way to determine when OOB data was read
from the network, but I can't find it right now.

> >                               Assuming nobody complains, I will commit it
> > tomorrow or the next day.
>
> no real complaints, but no reason not to address the minor issues
> below before committing

Some I'll fix, some I'll just comment on.  :-)

> > Index: server/mpm_common.c
> > ===================================================================
> > RCS file: /home/cvs/httpd-2.0/server/mpm_common.c,v
> > retrieving revision 1.47
> > diff -u -d -b -w -u -r1.47 mpm_common.c
> > --- server/mpm_common.c	2001/05/16 20:51:28	1.47
> > +++ server/mpm_common.c	2001/06/04 16:00:37
> > @@ -338,4 +338,61 @@
> >  }> >  #endif /* def NEED_INITGROUPS */
> >
> > +#ifdef AP_MPM_USES_POD
> > +static apr_file_t *pod_in = NULL, *pod_out = NULL;
> > +
> > +AP_DECLARE(apr_status_t) ap_mpm_pod_open(apr_pool_t *p)
> > +{
> > +    apr_file_t *pod;
> >
> > +    pod = apr_file_pipe_create(&pod_in, &pod_out, p);
> > +    apr_file_pipe_timeout_set(out, 0);
> > +    return APR_SUCCESS;
> > +}
> > +
> > +AP_DECLARE(apr_status_t) ap_mpm_pod_check(void)
> > +{
> > +    char c;
> > +    apr_size_t len = 1;
> > +    apr_status_t rv;
> > +
> > +    rv = apr_file_read(pod_in, &c, &len);
> > +
> > +    if ((rv == APR_SUCCESS) && (len == 1)) {
> > +        return APR_SUCCESS;
> > +    }
> > +    return 1;
> > +}
>
> This feels like a boolean routine.  Why not int/0/non-zero instead of
> apr_status_t/APR_SUCCESS/1?

My original thought had been to return the error code, but I was sick this
weekend, an just wanted to get something that works.  :-)  I am VERY
likely to add in the status code return.  That way, it is easier to log
errors.

> > +AP_DECLARE(void) ap_mpm_pod_close(void)
> > +{
> > +    apr_file_close(pod_out);
> > +    apr_file_close(pod_in);
> > +}
> > +
> > +AP_DECLARE(void) ap_mpm_pod_signal(apr_pool_t *p)
> > +{
> > +    apr_socket_t *sock = NULL;
> > +    apr_sockaddr_t *sa = NULL;
>
> init to NULL not necessary... this may make some folks think it is
> needed (as it is/was for some APR functions)

Force of habit, that will be fixed.

> > +
> > +    if ((rv = apr_file_write(pipe_of_death_out, &char_of_death, &one))
> > +                                 != APR_SUCCESS) {
> > +        if (APR_STATUS_IS_EINTR(rv)) continue;
> > +            ap_log_error(APLOG_MARK, APLOG_WARNING, rv, ap_server_conf,
> > +                         "write pipe_of_death");
> > +        }
> > +    }
> > +
> > +    apr_sockaddr_info_get(&sa, "127.0.0.1", APR_UNSPEC, ap_listeners->sd->port, 0, p);
> > +    apr_socket_create(&sock, sa->family, SOCK_STREAM, p);
> > +    apr_connect(sock, sa);
>
> very helpful to log errors from the last two calls, if only to know
> without a shadow of a doubt when debugging a shutdown problem that
> they worked 'cause otherwise you'd have seen the message in error_log
>
> crucial to close the socket here so you don't run out of file
> descriptors while executing the loop in ap_mpm_pod_killpg()...

Both good points.  As I said, this was minimal, because I did it in
between sleeping for hours at a time.  :-)

>
> > Index: server/mpm/prefork/prefork.c
>
> I didn't look your prefork changes in detail...
>
> SIGHUP processing in child processes should be SIG_IGN and SA_RESTART,
> right?

It is SIG_IGN, and I removed the the call to sigaddset for SIGWINCH.  I
THINK I got all the calls to SIGWINCH, but again, review is always useful.

> ---------/----------
>
> This feels like a reasonable the way to go but when this has been
> brought up before I've had some general worries about it that perhaps
> folks can dispel.  (Maybe I'm just mad because when I got the great
> idea to use connect() it wouldn't do what I wanted it to do :) )
>
> I remain at least a little concerned about using connect() because
>
> a) weird firewall-like rules on some possibly-weird systems may keep
>    the connect() from working; admin needs to be aware

We are connecting from localhost to localhost.  While weird firewall rules
may stop this, remember that we are connecting on a port that should be
accessible, because it is meant to accept connections.  :-)

> b) TCP/IP may be dead/dying and socket()+connect() won't work
>
>    I'm not sure how to kill apache on OS/390 when the stack goes away.
>
>    I was told previously by you and possibly others that I couldn't
>    teach the child process to tell the parent to get the @#$% out
>    because the stack is dead.  If the stack is dead, the user can tell
>    apache to go away but nothing will happen and the child processes
>    will (probably) continue looping and getting weird errors on
>    select() or accept().
>
>    This needs to be solved.  For me, simply recognizing the magic
>    error feedback and using the magic error status (CHILD_EXITFATAL?)
>    is sufficient.

Greg Ames stated quite emphatically, that if the stack dies on a 390
machine, the child process will wake up from select.  Once that happens,
the process will die, if for no other reason than that Apache dies when
select returns an error other than EINTR.

> c) We've invented a nice utility for MPMs but it won't work for MPMs
>    which want to parent process to decide which child should go away.
>    (I realize that isn't a current requirement.)

Yeah, I thought about this.  The only solution I have, is to have the
parent process write the pid of the process to die.  That would work, but
it can cause race-conditions that I don't want to think about today.  :-)

Ryan

_______________________________________________________________________________
Ryan Bloom                        	rbb@apache.org
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------