httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Greg Stein <gst...@lyra.org>
Subject Re: New graceful restart mechanism.
Date Mon, 04 Jun 2001 22:06:19 GMT
On Mon, Jun 04, 2001 at 09:20:46AM -0700, rbb@covalent.net wrote:
>...
> --- 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;

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).

> +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.

> +{
> +    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? :-)

>...
> +AP_DECLARE(apr_status_t) ap_mpm_pod_check(void)

Take a context.  (same for other functions)

> +{
> +    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.

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? ]

>...
> +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? :-)

>...
> +    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.

> +    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?

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.


The MPM changes seem fine, but I didn't review as closely.

Cheers,
-g

-- 
Greg Stein, http://www.lyra.org/

Mime
View raw message