Mailing-List: contact cvs-help@httpd.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@httpd.apache.org
Date: 15 Sep 2004 15:09:10 -0000
Message-ID: <20040915150910.25691.qmail@minotaur.apache.org>
From: clar@apache.org
To: httpd-2.0-cvs@apache.org
Subject: cvs commit: httpd-2.0/modules/experimental mod_cache.h
 mod_mem_cache.c

clar        2004/09/15 08:09:10

  Modified:    modules/experimental mod_cache.h mod_mem_cache.c
  Log:
  Fixed race condition when cleaning a cache object from multiple thread.
  
  Revision  Changes    Path
  1.49      +1 -2      httpd-2.0/modules/experimental/mod_cache.h
  
  Index: mod_cache.h
  ===================================================================
  RCS file: /home/cvs/httpd-2.0/modules/experimental/mod_cache.h,v
  retrieving revision 1.48
  retrieving revision 1.49
  diff -u -r1.48 -r1.49
  --- mod_cache.h	5 Aug 2004 19:12:34 -0000	1.48
  +++ mod_cache.h	15 Sep 2004 15:09:09 -0000	1.49
  @@ -172,8 +172,7 @@
       void *vobj;         /* Opaque portion (specific to the cache implementation) of the cache object */
       apr_size_t count;   /* Number of body bytes written to the cache so far */
       int complete;
  -    apr_uint32_t refcount;
  -    apr_size_t cleanup;
  +    apr_uint32_t refcount;  /* refcount and bit flag to cleanup object */
   };
   
   typedef struct cache_handle cache_handle_t;
  
  
  
  1.115     +28 -52    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.114
  retrieving revision 1.115
  diff -u -r1.114 -r1.115
  --- mod_mem_cache.c	26 Aug 2004 16:53:07 -0000	1.114
  +++ mod_mem_cache.c	15 Sep 2004 15:09:09 -0000	1.115
  @@ -88,6 +88,8 @@
   #define DEFAULT_MAX_STREAMING_BUFFER_SIZE 100000
   #define CACHEFILE_LEN 20
   
  +#define OBJECT_CLEANUP_BIT 0x00800000   /* flag to cleanup cache object */
  +
   /* Forward declarations */
   static int remove_entity(cache_handle_t *h);
   static apr_status_t store_headers(cache_handle_t *h, request_rec *r, cache_info *i);
  @@ -142,27 +144,25 @@
       return obj->key;
   }
   /** 
  - * callback to free an entry 
  - * There is way too much magic in this code. Right now, this callback
  - * is only called out of cache_insert() which is called under protection
  - * of the sconf->lock, which means that we do not (and should not)
  - * attempt to obtain the lock recursively. 
  + * function used to free an entry, used as a callback and as a local function. 
  + * There is way too much magic in this code. This callback
  + * has to be called under protection of the sconf->lock, which means that we 
  + * do not (and should not) attempt to obtain the lock recursively. 
    */
   static void memcache_cache_free(void*a)
   {
       cache_object_t *obj = (cache_object_t *)a;
  +    apr_uint32_t tmp_refcount = obj->refcount;
   
  -    /* Cleanup the cache object. Object should be removed from the cache
  -     * now. Increment the refcount before setting cleanup to avoid a race 
  -     * condition. A similar pattern is used in remove_url()
  -     */
  -    apr_atomic_inc32(&obj->refcount);
  -
  -    obj->cleanup = 1;
  -
  -    if (!apr_atomic_dec32(&obj->refcount)) {
  +    /* Cleanup the cache object. Object should be removed from the cache now. */
  +    if (!apr_atomic_read32(&obj->refcount)) {
           cleanup_cache_object(obj);
       }
  +    /* checking if there was a collision with another thread */
  +    else if(tmp_refcount != apr_atomic_cas32(&obj->refcount, tmp_refcount | OBJECT_CLEANUP_BIT, tmp_refcount)) {
  +        memcache_cache_free(obj);
  +    }
  +    return;
   }
   /*
    * functions return a 'negative' score since priority queues
  @@ -276,11 +276,11 @@
           }
           /* Remember, objects marked for cleanup are, by design, already
            * removed from the cache. remove_url() could have already
  -         * removed the object from the cache (and set obj->cleanup)
  +         * removed the object from the cache (and set the OBJECT_CLEANUP_BIT)
            */
  -        if (!obj->cleanup) {
  +        if (!(apr_atomic_read32(&obj->refcount) & OBJECT_CLEANUP_BIT)) {
               cache_remove(sconf->cache_cache, obj);
  -            obj->cleanup = 1;
  +            apr_atomic_set32(&obj->refcount, obj->refcount | OBJECT_CLEANUP_BIT);
           }
           if (sconf->lock) {
               apr_thread_mutex_unlock(sconf->lock);
  @@ -288,10 +288,8 @@
       }
   
       /* Cleanup the cache object */
  -    if (!apr_atomic_dec32(&obj->refcount)) {
  -        if (obj->cleanup) {
  -            cleanup_cache_object(obj);
  -        }
  +    if(apr_atomic_dec32(&obj->refcount) == OBJECT_CLEANUP_BIT) {
  +        cleanup_cache_object(obj);
       }
       return APR_SUCCESS;
   }
  @@ -314,11 +312,7 @@
       while (obj) {         
       /* Iterate over the cache and clean up each entry */  
       /* Free the object if the recount == 0 */
  -        apr_atomic_inc32(&obj->refcount);
  -        obj->cleanup = 1;
  -        if (!apr_atomic_dec32(&obj->refcount)) {
  -            cleanup_cache_object(obj);
  -        }
  +        memcache_cache_free(obj);
           obj = cache_pop(co->cache_cache);
       }
   
  @@ -415,7 +409,6 @@
       apr_atomic_set32(&obj->refcount, 1);
       mobj->total_refs = 1;
       obj->complete = 0;
  -    obj->cleanup = 0;
       obj->vobj = mobj;
       /* Safe cast: We tested < sconf->max_cache_object_size above */
       mobj->m_len = (apr_size_t)len;
  @@ -536,9 +529,9 @@
        * an object marked for cleanup is by design not in the
        * hash table.
        */
  -    if (!obj->cleanup) {
  +    if (!(apr_atomic_read32(&obj->refcount) & OBJECT_CLEANUP_BIT)) {
           cache_remove(sconf->cache_cache, obj);
  -        obj->cleanup = 1;
  +        apr_atomic_set32(&obj->refcount, obj->refcount | OBJECT_CLEANUP_BIT);
           ap_log_error(APLOG_MARK, APLOG_INFO, 0, NULL, "gcing a cache entry");
       }
   
  @@ -626,26 +619,12 @@
     
       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;
  -
  -        /* Refcount increment in this case MUST be made under 
  -         * protection of the lock 
  -         */
  -        apr_atomic_inc32(&obj->refcount);
  -        if (obj) {
  -            obj->cleanup = 1;
  -        }
  +        cache_remove(sconf->cache_cache, obj);        
  +        memcache_cache_free(obj);
       }
       if (sconf->lock) {
           apr_thread_mutex_unlock(sconf->lock);
       }
  -    if (obj) {
  -        if (!apr_atomic_dec32(&obj->refcount)) {
  -            cleanup_cache_object(obj);
  -        }
  -    }
       return OK;
   }
   
  @@ -908,8 +887,8 @@
                   if (sconf->lock) {
                       apr_thread_mutex_lock(sconf->lock);
                   }
  -                if (obj->cleanup) {
  -                    /* If obj->cleanup is set, the object has been prematurly 
  +                if ((apr_atomic_read32(&obj->refcount) & OBJECT_CLEANUP_BIT)) {
  +                    /* If OBJECT_CLEANUP_BIT is set, the object has been prematurly 
                        * ejected from the cache by the garbage collector. Add the
                        * object back to the cache. If an object with the same key is 
                        * found in the cache, eject it in favor of the completed obj.
  @@ -918,12 +897,9 @@
                         (cache_object_t *) cache_find(sconf->cache_cache, obj->key);
                       if (tmp_obj) {
                           cache_remove(sconf->cache_cache, tmp_obj);
  -                        tmp_obj->cleanup = 1;
  -                        if (!tmp_obj->refcount) {
  -                            cleanup_cache_object(tmp_obj);
  -                        }
  +                        memcache_cache_free(tmp_obj);
                       }
  -                    obj->cleanup = 0;
  +                    apr_atomic_set32(&obj->refcount, obj->refcount & ~OBJECT_CLEANUP_BIT);
                   }
                   else {
                       cache_remove(sconf->cache_cache, obj);