On Fri, May 23, 2008 at 12:28 PM, Nick Kew <[EMAIL PROTECTED]> wrote: > On Mon, 12 May 2008 20:22:09 +0100 > Nick Kew <[EMAIL PROTECTED]> 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
