2016-08-11 8:43 GMT+02:00 Luca Toscano <toscano.l...@gmail.com>: > > > 2016-08-09 15:02 GMT+02:00 Luca Toscano <toscano.l...@gmail.com>: > >> 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 :) >> > > 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). >
This patch might also miss another point, namely the calls to process_timeout_queue to manage Keep Alive timeouts, lingering closes and write completion. IIUC mpm-event explicitly process them after each wake up (every 100ms). Will work more on it, in the meantime thanks to all for the feedback! Luca
