httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From <...@covalent.net>
Subject Re: New graceful restart mechanism.
Date Mon, 04 Jun 2001 22:11:55 GMT

> > +#ifdef AP_MPM_USES_POD
> > +static apr_file_t *pod_in = NULL, *pod_out = NULL;
>
> Please use a context structure rather than globals. That will allow us to
> create a POD on a per-thread basis. Then you'll have your desired
> functionality of a parent telling a child, "hey *you*. please stop."
>
> An MPM can also choose to use just one POD for all of its children, and
> signal it N times (like you did in this patch).
>
> You could also use one POD for graceful restart and one for shutdown (let's
> say that an MPM is built to reset its state, rather than expect to die/spawn
> a thread).

I disagree.  By using a simple global variable, we make it much easier to
use this in an MPM.  I had originally done exactly what you suggested, and
I found that the code that needed to be added to the MPM was more complex
than I wanted it to be.  This change makes the pod completely opaque to
MPMs.  If they want to use it some other way, then they will need to
re-write the pod stuff anyway, and they can change the prototype, or just
keep it internal to the MPM.

> > +AP_DECLARE(apr_status_t) ap_mpm_pod_open(apr_pool_t *p)
>
> AP_DECLARE(apr_status_t) ap_mpm_pod_open(ap_pod_ctx **ctx, apr_pool_t *p)
>
> Take a pool. Return an ap_pod_ctx *.
>
> Note that the ap_pod_ctx can also keep a pool so the other functions won't> need to
accept one.

As I said above, I disagree.

> > +{
> > +    apr_file_t *pod;
> >
> > +    pod = apr_file_pipe_create(&pod_in, &pod_out, p);
> > +    apr_file_pipe_timeout_set(out, 0);
>
> "out" ... never heard of that variable. You sure you tested this? :-)

Compiled and tested.  This function as it happens had all sorts of
problems that the compiler missed.  I have fixed them all now.

> > +{
> > +    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;
>
> "1" is not a legal apr_status_t value. I would recommend one of two things
> for this function:
>
> 1) handle all errors internally (logging and whatnot) and return a boolean
>    on whether the caller should continue/die.

It is not easy to log in this function, because I don't really have enough
information.

> 2) return APR_SUCCESS, APR_ESHOULDDIE (or whatever), or an error and make
>    all callers deal with processing errors.
>
> I prefer option (1). A caller isn't going to know what to do with an error
> besides log it, so why return it to them?
>
> [ since the internals of ap_mpm_pod_check() are opaque, then when APR_EINTR
>   is returned, how can the MPM know what to do? ]

I have already fixed this, by creating a new apr_status_t for Apache to
use for this case.

> >...
> > +AP_DECLARE(void) ap_mpm_pod_signal(apr_pool_t *p)
> > +{
> > +    apr_socket_t *sock = NULL;
> > +    apr_sockaddr_t *sa = NULL;
> > +
> > +    if ((rv = apr_file_write(pipe_of_death_out, &char_of_death, &one))
>
> "pipe_of_death_out" "char_of_death" "one"  What are these variables?
>
> Are you really sure you tried this? :-)

I'm telling you, this all worked, I haven't got a clue why the damn build
isn't catching these, especially since I did a make clean;make.  This is
however beginning to bother me.

> >...
> > +    apr_sockaddr_info_get(&sa, "127.0.0.1", APR_UNSPEC, ap_listeners->sd->port,
0, p);
>
> Urk. This magic of looking into ap_listeners doesn't feel right. I don't
> have an immediate solution, but there must be some kind of abstraction for
> fetching the listening port.

Why?  We have the listening port in the ap_listeners list, so we should
use it.

> > +    apr_socket_create(&sock, sa->family, SOCK_STREAM, p);
> > +    apr_connect(sock, sa);
> > +}
>
> Besides the leak in file descriptors (as Jeff mentioned), we should also be
> careful of the pool. Is this pool going to disappear soon? Or if we're doing
> a restart, will it continue to be used?

This is pconf.  Therefore the pool will live until we restart, and then it
is cleared.

> Note that you also need to close the socket so the MPM will see that a
> request does not exist, thus allowing it to process the POD-signal right
> away. Otherwise, it will block on a read, waiting for input.

Already done.  Thanks.

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


Mime
View raw message