apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Henry Jen" <henry...@ztune.net>
Subject Re: apr_reslist semantics
Date Sat, 24 May 2008 04:18:05 GMT
On Fri, May 23, 2008 at 12:28 PM, Nick Kew <nick@webthing.com> 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.
>

Looks good.

Cheers,
Henry

Mime
View raw message