Re: cvs commit: httpd-2.0/server/mpm/worker worker.c

[Greg Ames <gr...@remulak.net>]

rbb@apache.org wrote:

>        dying = 1;
>   -    apr_lock_acquire(worker_thread_count_mutex);
>   -    worker_thread_count--;
>   -    if (worker_thread_count == 0) {
>   -        /* All the threads have exited, now finish the shutdown process
>   -         * by signalling the sigwait thread */
>   -        kill(ap_my_pid, SIGTERM);
>   -    }
>   -    apr_lock_release(worker_thread_count_mutex);
>   +    ap_scoreboard_image->parent[process_slot].quiescing = 1;
>   +    kill(ap_my_pid, SIGTERM);

moving the setting of quiescing here & simplifying the worker loop is
the way to go, for sure.  

But what about the kill?  I thought we had problems quite a while ago
trying to do about the same thing in threaded.  Don't we need to wait
for all the worker threads to exit before doing that, so we don't have
weird hangs? 
 
>        while (!workers_may_exit) {
>            ap_queue_pop(worker_queue, &csd, &ptrans);
>   +        if (!csd) {
>   +            continue;
>   +        }

what's this all about?  the only code that I see doing a push is
checking for !NULL

design concern about graceful restarts:  the queue has several
connections built up, and in comes a ! on the PoD.  It looks like we set
workers_may_exit, and the workers bail right away, before draining the
queue.  If true, we just lost connections.

Something else we may want to consider, once this becomes more stable: 
there's all those checks for workers_may_exit all over the accept loop. 
In threaded, we're pretty much forced to do that, because other threads
could be setting it at any time.  But here, the accept loop is single
threaded so some of the checks may be unneeded.

Greg

Re: cvs commit: httpd-2.0/server/mpm/worker worker.c

[Ryan Bloom <rb...@covalent.net>]

On Thursday 09 August 2001 23:29, Greg Ames wrote:
> rbb@apache.org wrote:
> >        dying = 1;
> >   -    apr_lock_acquire(worker_thread_count_mutex);
> >   -    worker_thread_count--;
> >   -    if (worker_thread_count == 0) {
> >   -        /* All the threads have exited, now finish the shutdown
> > process -         * by signalling the sigwait thread */
> >   -        kill(ap_my_pid, SIGTERM);
> >   -    }
> >   -    apr_lock_release(worker_thread_count_mutex);
> >   +    ap_scoreboard_image->parent[process_slot].quiescing = 1;
> >   +    kill(ap_my_pid, SIGTERM);
>
> moving the setting of quiescing here & simplifying the worker loop is
> the way to go, for sure.
>
> But what about the kill?  I thought we had problems quite a while ago
> trying to do about the same thing in threaded.  Don't we need to wait
> for all the worker threads to exit before doing that, so we don't have
> weird hangs?

No, we don't in this model, because the signal thread does an 
apr_thread_join to ensure that all of the workers are gone.

>
> >        while (!workers_may_exit) {
> >            ap_queue_pop(worker_queue, &csd, &ptrans);
> >   +        if (!csd) {
> >   +            continue;
> >   +        }
>
> what's this all about?  the only code that I see doing a push is
> checking for !NULL
>
> design concern about graceful restarts:  the queue has several
> connections built up, and in comes a ! on the PoD.  It looks like we set
> workers_may_exit, and the workers bail right away, before draining the
> queue.  If true, we just lost connections.

The design is simple.  All threads check the queue before exiting.  There is one race condition
right now that I still need to handle, but the design takes care of this, because
worker threads all sit waiting for the condition, and before they die, they check the queue.
Which, BTW is why the NULL is there.  If the queue is empty, then the worker will get
NULL, and it has to go back to sleep, or break out of the loop to die.

> Something else we may want to consider, once this becomes more stable:
> there's all those checks for workers_may_exit all over the accept loop.
> In threaded, we're pretty much forced to do that, because other threads
> could be setting it at any time.  But here, the accept loop is single
> threaded so some of the checks may be unneeded.

I'm getting to those.

_____________________________________________________________________________
Ryan Bloom                        	rbb@apache.org
Covalent Technologies			rbb@covalent.net
-----------------------------------------------------------------------------