httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Takashi Sato <taka...@lans-tv.com>
Subject Re: svn commit: r721952 - in /httpd/httpd/trunk: ./ modules/ modules/cluster/
Date Sat, 06 Dec 2008 14:29:09 GMT
On Mon, 01 Dec 2008 02:55:15 -0000
pquerna@apache.org wrote:

> Author: pquerna
> Date: Sun Nov 30 18:55:14 2008
> New Revision: 721952
> 
> URL: http://svn.apache.org/viewvc?rev=721952&view=rev
> Log:
> Add two new modules, originally written at Joost, to handle load balancing across
> multiple apache servers within the same datacenter.
> 
> mod_heartbeat generates multicast status messages with the current number of 
> clients connected, but the formated can easily be extended to include other things.
> 
> mod_heartmonitor collects these messages into a static file, which then can be 
> used for other modules to make load balancing decisions on.
> 
> Added:
>     httpd/httpd/trunk/modules/cluster/   (with props)
>     httpd/httpd/trunk/modules/cluster/Makefile.in   (with props)
>     httpd/httpd/trunk/modules/cluster/README.heartbeat
>     httpd/httpd/trunk/modules/cluster/README.heartmonitor
>     httpd/httpd/trunk/modules/cluster/config.m4
>     httpd/httpd/trunk/modules/cluster/mod_heartbeat.c   (with props)
>     httpd/httpd/trunk/modules/cluster/mod_heartmonitor.c   (with props)
> Modified:
>     httpd/httpd/trunk/CHANGES
>     httpd/httpd/trunk/modules/README
> 
[cut]
> Added: httpd/httpd/trunk/modules/cluster/mod_heartbeat.c
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/cluster/mod_heartbeat.c?rev=721952&view=auto
> ==============================================================================
> --- httpd/httpd/trunk/modules/cluster/mod_heartbeat.c (added)
> +++ httpd/httpd/trunk/modules/cluster/mod_heartbeat.c Sun Nov 30 18:55:14 2008
> @@ -0,0 +1,354 @@
> +/* Licensed to the Apache Software Foundation (ASF) under one or more
[cut]
> +typedef struct hb_ctx_t
> +{
> +    int active;
> +    apr_sockaddr_t *mcast_addr;
> +    int server_limit;
> +    int thread_limit;
> +    int status;

[cut]
> +
> +static void *hb_worker(apr_thread_t *thd, void *data)

Don't this need to be APR_THREAD_FUNC?

> +{
> +    hb_ctx_t *ctx = (hb_ctx_t *) data;
> +    apr_status_t rv;
> +
> +    apr_pool_t *pool = apr_thread_pool_get(thd);
> +    apr_pool_tag(pool, "heartbeat_worker");
> +    ctx->status = 0;

The meaning of "status zero" is unclear.

[cut]
> +static void start_hb_worker(apr_pool_t *p, hb_ctx_t *ctx)
> +{
> +    apr_status_t rv;
> +
> +    rv = apr_thread_mutex_create(&ctx->start_mtx, APR_THREAD_MUTEX_UNNESTED,
> +                                 p);
> +
> +    if (rv) {
> +        ap_log_error(APLOG_MARK, APLOG_CRIT, rv, NULL,
> +                     "Heartbeat: apr_thread_cond_create failed");
> +        ctx->status = rv;
> +        return;
> +    }

status is apr_status_t?

> +
> +    apr_thread_mutex_lock(ctx->start_mtx);
> +
> +    apr_pool_cleanup_register(p, ctx, hb_pool_cleanup, apr_pool_cleanup_null);
> +
> +    rv = apr_thread_create(&ctx->thread, NULL, hb_worker, ctx, p);
> +    if (rv) {
> +        apr_pool_cleanup_kill(p, ctx, hb_pool_cleanup);
> +        ap_log_error(APLOG_MARK, APLOG_CRIT, rv, NULL,
> +                     "Heartbeat: apr_thread_create failed");
> +        ctx->status = rv;
> +    }

Same above.

> +
> +    apr_thread_mutex_lock(ctx->start_mtx);
> +    apr_thread_mutex_unlock(ctx->start_mtx);
> +    apr_thread_mutex_destroy(ctx->start_mtx);
> +}
> +
> +static void hb_child_init(apr_pool_t *p, server_rec *s)
> +{
> +    hb_ctx_t *ctx = ap_get_module_config(s->module_config, &heartbeat_module);
> +
> +    apr_proc_mutex_child_init(&ctx->mutex, ctx->mutex_path, p);
> +
> +    ctx->status = -1;

I don't like this. "status -1" is unclear.

> +
> +    if (ctx->active) {
> +        start_hb_worker(p, ctx);
> +        if (ctx->status != 0) {

Same above.
Why not change the type of hb_ctx_t::status to apr_status_t ?

[cut]
> +static const char *cmd_hb_address(cmd_parms *cmd,
> +                                  void *dconf, const char *addr)
> +{
> +    apr_status_t rv;
> +    char *host_str;
> +    char *scope_id;
> +    apr_port_t port = 0;
> +    apr_pool_t *p = cmd->pool;
> +    hb_ctx_t *ctx =
> +        (hb_ctx_t *) ap_get_module_config(cmd->server->module_config,
> +                                          &heartbeat_module);
> +    const char *err = ap_check_cmd_context(cmd, GLOBAL_ONLY);
> +
> +    if (err != NULL) {
> +        return err;
> +    }
> +
> +    ctx->active = 1;
> +
> +    rv = apr_parse_addr_port(&host_str, &scope_id, &port, addr, p);

cmd->temp_pool is better than cmd->pool.

> +
> +    if (rv) {
> +        return "HeartbeatAddress: Unable to parse address.";
> +    }
> +
> +    if (host_str == NULL) {
> +        return "HeartbeatAddress: No host provided in address";
> +    }
> +
> +    if (port == 0) {
> +        return "HeartbeatAddress: No port provided in address";
> +    }
> +
> +    rv = apr_sockaddr_info_get(&ctx->mcast_addr, host_str, APR_INET, port, 0,
> +                               p);
> +
> +    if (rv) {
> +        return "HeartbeatAddress: apr_sockaddr_info_get failed.";
> +    }
> +
> +    const char *tmpdir = NULL;
> +    rv = apr_temp_dir_get(&tmpdir, p);

Same above.

> +    if (rv) {
> +        return "HeartbeatAddress: unable to find temp directory.";
> +    }
> +
> +    char *path = apr_pstrcat(p, tmpdir, "/hb-tmp.XXXXXX", NULL);

Same above.

[cut]
> Added: httpd/httpd/trunk/modules/cluster/mod_heartmonitor.c
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/cluster/mod_heartmonitor.c?rev=721952&view=auto
> ==============================================================================
> --- httpd/httpd/trunk/modules/cluster/mod_heartmonitor.c (added)
> +++ httpd/httpd/trunk/modules/cluster/mod_heartmonitor.c Sun Nov 30 18:55:14 2008
> @@ -0,0 +1,551 @@
> +/* Licensed to the Apache Software Foundation (ASF) under one or more
> + * contributor license agreements.  See the NOTICE file distributed with

[cut]

> +typedef struct hm_ctx_t
> +{
> +    int active;
> +    const char *storage_path;
> +    apr_proc_mutex_t *mutex;
> +    const char *mutex_path;
> +    apr_sockaddr_t *mcast_addr;
> +    int status;

Similar to mod_heartbeat.c

> +    int keep_running;
> +    apr_thread_mutex_t *start_mtx;
> +    apr_thread_t *thread;
> +    apr_socket_t *sock;
> +    apr_pool_t *p;
> +    apr_hash_t *servers;
> +} hm_ctx_t;
> +
[cut]
> +static void *hm_worker(apr_thread_t *thd, void *data)

Don't this need to be APR_THREAD_FUNC?

> +{
> +    hm_ctx_t *ctx = (hm_ctx_t *) data;
> +    apr_status_t rv;
> +
> +    ctx->p = apr_thread_pool_get(thd);
> +    ctx->status = 0;
> +    ctx->keep_running = 1;
> +    apr_thread_mutex_unlock(ctx->start_mtx);
> +
> +    while (ctx->keep_running) {
> +        rv = apr_proc_mutex_trylock(ctx->mutex);
> +        if (rv == APR_SUCCESS) {
> +            break;
> +        }
> +        apr_sleep(apr_time_from_msec(200));
> +    }
> +
> +    rv = hm_listen(ctx);
> +
> +    if (rv) {
> +        ctx->status = rv;
> +        ap_log_error(APLOG_MARK, APLOG_CRIT, rv, NULL,
> +                     "Heartmonitor: Unable to listen for connections!");
> +        apr_proc_mutex_unlock(ctx->mutex);
> +        apr_thread_exit(ctx->thread, rv);
> +        return NULL;
> +    }
> +
> +
> +    apr_time_t last = apr_time_now();
> +    while (ctx->keep_running) {
> +        int n;
> +        apr_pool_t *p;
> +        apr_pollfd_t pfd;
> +        apr_interval_time_t timeout;
> +        apr_pool_create(&p, ctx->p);
> +
> +        apr_time_t now = apr_time_now();
> +
> +        if (apr_time_sec((now - last)) > 5) {
> +            hm_update_stats(ctx, p);
> +            apr_pool_clear(p);
> +            last = now;
> +        }
> +
> +        pfd.desc_type = APR_POLL_SOCKET;
> +        pfd.desc.s = ctx->sock;
> +        pfd.p = p;
> +        pfd.reqevents = APR_POLLIN;
> +
> +        timeout = apr_time_from_sec(1);
> +
> +        rv = apr_poll(&pfd, 1, &n, timeout);
> +
> +        if (!ctx->keep_running) {
> +            break;
> +        }

The parent of p is ctx->p, which is child's process pool,
so this is not a memory leak. 
But to be clear IMHO it should call apr_pool_destroy.
Is it extra care?

[cut]
> +static const char *cmd_hm_storage(cmd_parms *cmd,
> +                                  void *dconf, const char *path)
> +{
> +    apr_pool_t *p = cmd->pool;
> +    hm_ctx_t *ctx =
> +        (hm_ctx_t *) ap_get_module_config(cmd->server->module_config,
> +                                          &heartmonitor_module);
> +    const char *err = ap_check_cmd_context(cmd, GLOBAL_ONLY);
> +
> +    if (err != NULL) {
> +        return err;
> +    }
> +
> +    ctx->storage_path = ap_server_root_relative(p, path);
> +    ctx->mutex_path =
> +        ap_server_root_relative(p, apr_pstrcat(p, path, ".hm-lock", NULL));

cmd->temp_pool is better than "p" for apr_pstrcat.

> +
> +    return NULL;
> +}
> +
> +static const char *cmd_hm_listen(cmd_parms *cmd,
> +                                 void *dconf, const char *mcast_addr)
> +{
> +    apr_status_t rv;
> +    char *host_str;
> +    char *scope_id;
> +    apr_port_t port = 0;
> +    apr_pool_t *p = cmd->pool;
> +    hm_ctx_t *ctx =
> +        (hm_ctx_t *) ap_get_module_config(cmd->server->module_config,
> +                                          &heartmonitor_module);
> +    const char *err = ap_check_cmd_context(cmd, GLOBAL_ONLY);
> +
> +    if (err != NULL) {
> +        return err;
> +    }
> +
> +    ctx->active = 1;
> +
> +    rv = apr_parse_addr_port(&host_str, &scope_id, &port, mcast_addr, p);

cmd->temp_pool is better than "p".


Regards,
Takashi
-- 
Takashi Sato <takashi@lans-tv.com>

Mime
View raw message