httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jean-Jacques Clar" <JJC...@novell.com>
Subject Re: cvs commit: httpd-2.0/modules/experimental mod_mem_cache.c
Date Thu, 08 Jan 2004 21:24:30 GMT
Bill,
 
The patch you committed is only for 21285.
 
My bet for 21287 (no mutex lock protection in decrement_refcount).
from the bug description:
"There are no mutex lock protection in decrement_refcount if it is
defined USE_ATOMICS.

I think you simply forgot the mutex in function decrement_refcount. 
There is a race condition in the cleanup_cache_object when two threads

are trying to clean-up the same object which is no longer referenced in
the cache."

The changes were done to protect calls to cleanup_cache_object() w/o

a lock protection, but you are right the atomic inc/dec on refcount 
seems to do it safely.
 
Looks like 21287 it is not a valid defect based on your explanation.
How do I rev back my changes?
 
JJ
>>> bill@wstoddard.com 1/8/2004 8:23:26 AM >>>

Here is the patch that fill fix the problem reported by 21287

http://cvs.apache.org/viewcvs.cgi/httpd-2.0/modules/experimental/mod_mem_cache.c?r1=1.99&r2=1.100

Bill
> Hi Jean-Jacques,
> What specific problem is this patch correcting? I committed a fix for

> 21287 prior to the holidays. The idea behind using atomic operators
on 
> refcount is to avoid the need to acquire the mutex when 
> incrementing/decrementing refcount.
> 
> Bill
> 
> clar@apache.org wrote:
> 
>> clar        2004/01/07 11:35:16
>>
>>   Modified:    modules/experimental mod_mem_cache.c
>>   Log:
>>   Fix for  Bug 21287.
>>   Patch extracted and modified from attachment in Bug 21285.
>>   The submitted patch was combining fix for 21285 and 21287.
>>   It is extending the mutex protection in decrement_refcount() and 
>> remove_url().
>>     Revision  Changes    Path
>>   1.103     +9 -26    
httpd-2.0/modules/experimental/mod_mem_cache.c
>>     Index: mod_mem_cache.c
>>  
===================================================================
>>   RCS file:
/home/cvs/httpd-2.0/modules/experimental/mod_mem_cache.c,v
>>   retrieving revision 1.102
>>   retrieving revision 1.103
>>   diff -u -r1.102 -r1.103
>>   --- mod_mem_cache.c    6 Jan 2004 21:57:50 -0000    1.102
>>   +++ mod_mem_cache.c    7 Jan 2004 19:35:16 -0000    1.103
>>   @@ -348,33 +348,26 @@
>>                cache_remove(sconf->cache_cache, obj);
>>                obj->cleanup = 1;
>>            }
>>   -        if (sconf->lock) {
>>   -            apr_thread_mutex_unlock(sconf->lock);
>>   -        }
>>   +    }
>>   +    else if (sconf->lock) {
>>   +        apr_thread_mutex_lock(sconf->lock);
>>        }
>>           /* Cleanup the cache object */
>>    #ifdef USE_ATOMICS
>>   -    if (!apr_atomic_dec32(&obj->refcount)) {
>>   -        if (obj->cleanup) {
>>   -            cleanup_cache_object(obj);
>>   -        }
>>   -    }
>>   +    if (!apr_atomic_dec32(&obj->refcount) && (obj->cleanup))
{
>>    #else
>>   -    if (sconf->lock) {
>>   -        apr_thread_mutex_lock(sconf->lock);
>>   -    }
>>        obj->refcount--;
>>        /* If the object is marked for cleanup and the refcount
>>         * has dropped to zero, cleanup the object
>>         */
>>        if ((obj->cleanup) && (!obj->refcount)) {
>>   +#endif
>>            cleanup_cache_object(obj);
>>        }
>>        if (sconf->lock) {
>>            apr_thread_mutex_unlock(sconf->lock);
>>        }
>>   -#endif
>>        return APR_SUCCESS;
>>    }
>>    static apr_status_t cleanup_cache_mem(void *sconfv)
>>   @@ -739,35 +732,25 @@
>>             obj = cache_find(sconf->cache_cache, key);             
if 
>> (obj) {
>>   -        mem_cache_object_t *mobj;
>>            cache_remove(sconf->cache_cache, obj);
>>   -        mobj = (mem_cache_object_t *) obj->vobj;
>>       #ifdef USE_ATOMICS
>>            /* Refcount increment in this case MUST be made under 
>>             * protection of the lock             */
>>            apr_atomic_inc32(&obj->refcount);
>>   +        obj->cleanup = 1;
>>   +        if (!apr_atomic_dec32(&obj->refcount)) {
>>    #else
>>   +        obj->cleanup = 1;
>>            if (!obj->refcount) {
>>   -            cleanup_cache_object(obj);
>>   -            obj = NULL;
>>   -        }
>>    #endif
>>   -        if (obj) {
>>   -            obj->cleanup = 1;
>>   +            cleanup_cache_object(obj);
>>            }
>>        }
>>        if (sconf->lock) {
>>            apr_thread_mutex_unlock(sconf->lock);
>>        }
>>   -#ifdef USE_ATOMICS
>>   -    if (obj) {
>>   -        if (!apr_atomic_dec32(&obj->refcount)) {
>>   -            cleanup_cache_object(obj);
>>   -        }
>>   -    }
>>   -#endif
>>        return OK;
>>    }
>>         
> 
> 
> 




Mime
View raw message