Re: cvs commit: httpd-2.0/modules/experimental mod_mem_cache.c

[Jean-Jacques Clar]

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 >>> [EMAIL PROTECTED] 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 > > [EMAIL PROTECTED] 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; >>    } >>         > > >