2016-08-09 15:02 GMT+02:00 Luca Toscano <[email protected]>: > Thanks a lot for the feedback, trying to answer to everybody: > > 2016-08-05 15:19 GMT+02:00 Jim Jagielski <[email protected]>: > >> 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 <[email protected]>: > >> On Fri, Aug 5, 2016 at 3:19 PM, Yann Ylavic <[email protected]> wrote: >> > Hi Luca, >> > >> > On Fri, Aug 5, 2016 at 2:58 PM, Luca Toscano <[email protected]> >> wrote: >> >> >> >> 2016-08-04 17:56 GMT+02:00 Luca Toscano <[email protected]>: >> >>> >> >>> 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 :) >
I applied Yann's suggestions and added some checks to allow wakeable and non wakeable listeners: http://apaste.info/mke This is a very high level prototype, please let me know if you have more suggestions (of course the discussion around testing and reliability of APR_POLLSET_WAKEABLE still holds). Thanks! Regards, Luca
