apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Ruediger Pluem <rpl...@apache.org>
Subject Re: apr_reslist semantics
Date Fri, 23 May 2008 21:27:24 GMT


On 05/23/2008 09:28 PM, Nick Kew wrote:
> On Mon, 12 May 2008 20:22:09 +0100
> Nick Kew <nick@webthing.com> wrote:
> 
>> In https://issues.apache.org/bugzilla/show_bug.cgi?id=42841 ,
>> Tom points out an issue that gives problems with MySQL
>> (and possibly other DBD drivers) and suggests that a change
>> to apr_reslist semantics would fix it.  Tom also attaches
>> a patch implementing his proposed change.
> 
>> [chop]
> 
> After far too long, I've revisited this issue and Tom's patch.
> 
> It seems to me we can get *both* semantics at once:
> 
> * Keep reslist_maint as is, preserving its existing semantics
> * Check the idle time of a resource in apr_reslist_acquire,
>   and kill a resource if it's too old.
> 
> There are other considerations, but a minimal effective patch
> looks like:
> 
> ===================================================================
> --- apr_reslist.c       (revision 659614)
> +++ apr_reslist.c       (working copy)
> @@ -292,13 +292,22 @@
>  {
>      apr_status_t rv;
>      apr_res_t *res;
> +    apr_time_t now = apr_time_now();
>  
>      apr_thread_mutex_lock(reslist->listlock);
>      /* If there are idle resources on the available list, use
>       * them right away. */
> -    if (reslist->nidle > 0) {
> +    while (reslist->nidle > 0) {
>          /* Pop off the first resource */
>          res = pop_resource(reslist);
> +        if (reslist->ttl && (now - res->freed >= reslist->ttl))
{
> +            /* this res is expired - kill it */
> +            rv = destroy_resource(reslist, res);
> +            if (rv != APR_SUCCESS) {
> +                apr_thread_mutex_unlock(reslist->listlock);
> +                return rv;  /* FIXME: this might cause unnecessary
> fails */
> +            }
> +            continue;
> +        }
>          *resource = res->opaque;
>          free_container(reslist, res);
>          apr_thread_mutex_unlock(reslist->listlock);
> @@ -306,8 +315,7 @@
>      }
>      /* If we've hit our max, block until we're allowed to create
>       * a new one, or something becomes free. */
> -    else while (reslist->ntotal >= reslist->hmax
> -                && reslist->nidle <= 0) {
> +    while (reslist->ntotal >= reslist->hmax && reslist->nidle <=
0) {
>          if (reslist->timeout) {
>              if ((rv = apr_thread_cond_timedwait(reslist->avail, 
>                  reslist->listlock, reslist->timeout)) != APR_SUCCESS) {
> 
> It's not perfect, but it has the advantage of neither disturbing
> the API/ABI, nor breaking promises to existing apps.

 From first glance this looks good.

Regards

RĂ¼diger


Mime
View raw message