httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Justin Erenkrantz <jerenkra...@apache.org>
Subject Re: [PATCH] don't accept more connections than idle workers
Date Sat, 27 Apr 2002 01:56:33 GMT
On Fri, Apr 26, 2002 at 05:07:29PM -0700, Aaron Bannert wrote:

Comments inline.

> Index: server/mpm/worker/worker.c
> ===================================================================
> RCS file: /home/cvs/httpd-2.0/server/mpm/worker/worker.c,v
> retrieving revision 1.117
> diff -u -u -r1.117 worker.c
> --- server/mpm/worker/worker.c	18 Apr 2002 17:46:20 -0000	1.117
> +++ server/mpm/worker/worker.c	26 Apr 2002 23:46:32 -0000
> @@ -173,6 +173,7 @@
>  static int num_listensocks = 0;
>  static int resource_shortage = 0;
>  static fd_queue_t *worker_queue;
> +static fd_queue_info_t *worker_queue_info;
>  
>  /* The structure used to pass unique initialization info to each thread */
>  typedef struct {
> @@ -299,6 +300,7 @@
>      if (mode == ST_UNGRACEFUL) {
>          workers_may_exit = 1;
>          ap_queue_interrupt_all(worker_queue);
> +        ap_queue_info_term(worker_queue_info);
>      }
>  }
>  
> @@ -693,6 +695,20 @@
>          }
>          if (listener_may_exit) break;
>  
> +        rv = ap_queue_info_wait_for_idler(worker_queue_info);
> +        if (APR_STATUS_IS_EOF(rv)) {
> +            break; /* we've been signaled to die now */
> +        }
> +        else if (rv != APR_SUCCESS) {
> +            ap_log_error(APLOG_MARK, APLOG_EMERG, rv, ap_server_conf,
> +                         "apr_queue_info_wait failed. Attempting to shutdown "
> +                         "process gracefully.");
> +            signal_threads(ST_GRACEFUL);
> +            break;
> +        }

I'm slightly confused here.  When we see EOF does that mean
that someone else has already called signal_threads for
ungraceful shutdowns?

> Index: server/mpm/worker/fdqueue.c
> ===================================================================
> RCS file: /home/cvs/httpd-2.0/server/mpm/worker/fdqueue.c,v
> retrieving revision 1.15
> diff -u -u -r1.15 fdqueue.c
> --- server/mpm/worker/fdqueue.c	26 Apr 2002 17:13:51 -0000	1.15
> +++ server/mpm/worker/fdqueue.c	26 Apr 2002 23:46:33 -0000
> @@ -58,6 +58,114 @@
>  
>  #include "fdqueue.h"
>  
> +struct fd_queue_info_t {
> +    int idlers;
> +    apr_thread_mutex_t *idlers_mutex;
> +    apr_thread_cond_t *wait_for_idler;
> +    int terminated;
> +};
> +
> +static apr_status_t queue_info_cleanup(void *data_)
> +{
> +    fd_queue_info_t *qi = data_;
> +    apr_thread_cond_destroy(qi->wait_for_idler);
> +    apr_thread_mutex_destroy(qi->idlers_mutex);
> +    return APR_SUCCESS;
> +}
> +
> +apr_status_t ap_queue_info_create(fd_queue_info_t **queue_info,
> +                                  apr_pool_t *pool)
> +{
> +    apr_status_t rv;
> +    fd_queue_info_t *qi;
> +
> +    qi = apr_palloc(pool, sizeof(*qi));
> +    memset(qi, 0, sizeof(*qi));

Why not apr_pcalloc?

> +
> +    rv = apr_thread_mutex_create(&qi->idlers_mutex, APR_THREAD_MUTEX_DEFAULT,
> +                                 pool);
> +    if (rv != APR_SUCCESS) {
> +        return rv;
> +    }
> +    rv = apr_thread_cond_create(&qi->wait_for_idler, pool);
> +    if (rv != APR_SUCCESS) {
> +        return rv;
> +    }
> +    apr_pool_cleanup_register(pool, qi, queue_info_cleanup,
> +                              apr_pool_cleanup_null);
> +
> +    *queue_info = qi;
> +
> +    return APR_SUCCESS;
> +}
> +
> +apr_status_t ap_queue_info_set_idle(fd_queue_info_t *queue_info)
> +{
> +    apr_status_t rv;
> +    rv = apr_thread_mutex_lock(queue_info->idlers_mutex);
> +    if (rv != APR_SUCCESS) {
> +        return rv;
> +    }
> +    AP_DEBUG_ASSERT(queue_info->idlers >= 0);
> +    if (queue_info->idlers++ == 0) {
> +        /* Only signal if we had no idlers before. */
> +        apr_thread_cond_signal(queue_info->wait_for_idler);
> +    }
> +    rv = apr_thread_mutex_unlock(queue_info->idlers_mutex);
> +    if (rv != APR_SUCCESS) {
> +        return rv;
> +    }
> +    return APR_SUCCESS;
> +}
> +
> +apr_status_t ap_queue_info_wait_for_idler(fd_queue_info_t *queue_info)
> +{
> +    apr_status_t rv;
> +    rv = apr_thread_mutex_lock(queue_info->idlers_mutex);
> +    if (rv != APR_SUCCESS) {
> +        return rv;
> +    }
> +    AP_DEBUG_ASSERT(queue_info->idlers >= 0);
> +    while ((queue_info->idlers == 0) && (!queue_info->terminated)) {
> +        rv = apr_thread_cond_wait(queue_info->wait_for_idler,
> +                                  queue_info->idlers_mutex);
> +        if (rv != APR_SUCCESS) {
> +            rv = apr_thread_mutex_unlock(queue_info->idlers_mutex);
> +            if (rv != APR_SUCCESS) {
> +                return rv;
> +            }
> +            return rv;
> +        }

How about just return rv in this case?  =)  Greg is always
pointing these out, so I'll learn from his reviews.

Otherwise, this looks good conceptually and doesn't suffer from
the yield() of FirstBill's patch.  I will try to test it tonight
or tomorrow on my Linux box.  Hopefully, we should be able to
bump this into .36 - Sander permitting.  -- justin

Mime
View raw message