- I think it is good to fix this behavior, using only the global keepalive timeout was definitely a choice I (?) made when doing it.
- Skiplists seem generally acceptable for storing this data. Alternatively a BTree with in order iteration would be competitive.. but, meh, either will be fine. - I'd love if we could seek ways to unify the timeout queues inside the Event MPM. Right now we have: * keepalive_q * write_completion_q * linger_q * short_linger_q Maybe making a single structure for all timeouts, with a baton / type enum for any extra data or the function to call? For example `write_completion_q` is mapped to global server TimeOut directive.. so same bug as KeepAliveTimeOut. Making just the keepalive queue a skiplist, and leaving the others... seems less than optimal in my view. - No comment on fixing APR Skiplists :) On Thu, May 22, 2014 at 5:04 AM, Yann Ylavic <[email protected]> wrote: > Hello, > > while working on > https://issues.apache.org/bugzilla/show_bug.cgi?id=56226 for a > possible way to use vhost's KeepAliveTimeout with mpm_event (by using > a skiplist instead of the actual keepalive queue), I realized that > apr_skiplists do not accept multiple values per key (unlike apr_table > for example, or std::multimap). > > AFAICT this is only an implementation choice, which could be changed > with something like : > Index: tables/apr_skiplist.c > =================================================================== > --- tables/apr_skiplist.c (revision 1595631) > +++ tables/apr_skiplist.c (working copy) > @@ -406,11 +406,7 @@ APR_DECLARE(apr_skiplistnode *) apr_skiplist_inser > if (m->next) { > compared = comp(data, m->next->data); > } > - if (compared == 0) { > - free(stack); /* OK. was malloc'ed */ > - return 0; > - } > - if ((m->next == NULL) || (compared < 0)) { > + if (compared < 0) { > if (ch <= nh) { > /* push on stack */ > stack[stacki++] = m; > > (probably with a new apr_skiplist_set_multi() which would make it > controlable). > > The skiplist's key used for timers by mpm_event is the expiration time > (in microseconds), I think the issue is multiple timers created at the > same time (which may happen when different threads request a timer, > eg. with event_register_[timed|socket]_callback). > In that case, none but the first one will be inserted in the skiplist > (and hence known by listener). > > Shouldn't skiplists accept multiple values for the same key by default? > > For 56226, if we were to replace the actual keepalive queue with a > skiplist, the proposed patch suffers the same issue. > If however the current behaviour (and speed) of using the base > server's KeepaliveTimeout for all the vhosts is a choice, I think it > should at least be documented on the KeepAliveTimeout directive, as a > limitation for mpm_event. > > Regards, > Yann.
