httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Paul Querna <p...@querna.org>
Subject Re: apr_skiplist (current) implementation wrt mpm_event (timers, keepalives?)
Date Thu, 22 May 2014 15:13:23 GMT
- 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 <ylavic.dev@gmail.com> 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.

Mime
View raw message