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
