On Fri, Apr 26, 2002 at 06:56:33PM -0700, Justin Erenkrantz wrote:
> > + 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?
Yes.
> > + qi = apr_palloc(pool, sizeof(*qi));
> > + memset(qi, 0, sizeof(*qi));
>
> Why not apr_pcalloc?
I've made a habit of not using calloc. palloc+memset is faster. In
this case, when it will only be used once, it doesn't make much of
a difference.
> > + 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.
Actually, I did this incorrectly, so thanks for pointing this out.
I'm being really over-aggressive in catching errors, but better to be
safe than sorry. What I should have done is used a different identifier
for the nested apr_status_t.
-aaron
