Thanks a lot for the feedback, trying to answer to everybody: 2016-08-05 15:19 GMT+02:00 Jim Jagielski <j...@jagunet.com>:
> If APR_POLLSET_WAKEABLE was more universal and, therefore, > more widely tested, I'd be +1... as it is, let's see what > the feedback is. Definitely, we should find a good way to test this if we'll reach consensus on a patch. Maybe some stress tests plus "incubation" in trunk for a while could be good enough, but I agree that it would be an important change to make with extreme care. 2016-08-05 15:26 GMT+02:00 Yann Ylavic <ylavic....@gmail.com>: > On Fri, Aug 5, 2016 at 3:19 PM, Yann Ylavic <ylavic....@gmail.com> wrote: > > Hi Luca, > > > > On Fri, Aug 5, 2016 at 2:58 PM, Luca Toscano <toscano.l...@gmail.com> > wrote: > >> > >> 2016-08-04 17:56 GMT+02:00 Luca Toscano <toscano.l...@gmail.com>: > >>> > >>> Would it be possible to avoid them adding APR_POLLSET_WAKEABLE to the > >>> event_pollset flags and calling apr_pollset_wakeup right after > >>> apr_skiplist_insert? > > > > That might be possible, but there is more than apr_skiplist_insert() > > to handle I'm afraid, see below. > > > >> > >> Very high level idea: http://apaste.info/MKE > > > > The timeout_interval also handles the listener_may_exit (and > > connection_count == 0 in graceful mode), your patch seems not handle > > them... > You are right, and IIUC these are the use cases to cover: 1) listener thread(s) blocked on apr_pollset_poll and a call to wakeup_listener (that afaiu is the only one setting listener_may_exit to 1); 2) listener thread(s) blocked on apr_pollset_poll, listener_may_exit = 1 and connection_count still greater than zero. The last connection cleanup (that calls decrement_connection_count registered previously with apr_pool_cleanup_register) should be able to wake up the listener that in turns will be able to execute the break in the following block: if (listener_may_exit) { close_listeners(process_slot, &closed); if (terminate_mode == ST_UNGRACEFUL || apr_atomic_read32(&connection_count) == 0) break; } I would also add another one, namely if the pollset create does not support APR_POLLSET_WAKEABLE. This would mean that in absence of apr_pollset_wakeup we should rely on apr_pollset_poll with the 100 ms timeout, keeping it as "fallback" to ensure compatibility with most of the systems. > > The listener_may_exit case may be easily handled in wakeup_listener > > (one more apr_pollset_wakeup() there), however the connection_count > > looks more complicated. > > > > IOW, not sure something like the below would be enough... > > > > Index: server/mpm/event/event.c > > =================================================================== > > --- server/mpm/event/event.c (revision 1755288) > > +++ server/mpm/event/event.c (working copy) > > @@ -484,6 +484,7 @@ static void close_worker_sockets(void) > > static void wakeup_listener(void) > > { > > listener_may_exit = 1; > > + apr_pollset_wakeup(event_pollset); > > if (!listener_os_thread) { > > /* XXX there is an obscure path that this doesn't handle > perfectly: > > * right after listener thread is created but before > > Or rather, in wakeup_listener(): > > @@ -493,6 +493,9 @@ static void wakeup_listener(void) > return; > } > > + /* unblock the listener if it's waiting for a timer */ > + apr_pollset_wakeup(event_pollset); > + > /* unblock the listener if it's waiting for a worker */ > ap_queue_info_term(worker_queue_info); > > _ > > > @@ -696,7 +697,9 @@ static apr_status_t decrement_connection_count(voi > > default: > > break; > > } > > - apr_atomic_dec32(&connection_count); > > + if (!apr_atomic_dec32(&connection_count) && listener_may_exit) { > > + apr_pollset_wakeup(event_pollset); > > + } > > return APR_SUCCESS; > > } > I like these suggestions, they look good! (This assuming that what I have written above is correct, otherwise I didn't get them :) Regards, Luca
