Jean-Jacques Clar wrote:
Should I then go ahead and commit my patch to the 2.1 tree?
This section from decrement_refcount() makes me nervous:
- if (!obj->cleanup) {
+ if (!(apr_atomic_read(&obj->refcount) & OBJECT_CLEANUP_BIT)) {
cache_remove(sconf->cache_cache, obj);
- obj->cleanup = 1;
+ apr_atomic_set(&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_dec(&obj->refcount)) {
- if (obj->cleanup) {
- cleanup_cache_object(obj);
- }
+ if(apr_atomic_dec(&obj->refcount) == OBJECT_CLEANUP_BIT) {
+ cleanup_cache_object(obj);
}It's the use of apr_atomic_set(). That line will take a snapshot of the value of obj->refcount at some point in time, OR on the OBJECT_CLEANUP_BIT, then unconditionally store the result back into obj->refcount.
But there could be an unlocked caller trying to simultaneously decrement the refcount on another thread, right? I say that because of the use of apr_atomic_dec() a few lines later. If so, the apr_atomic_set could loose the decrement from the other thread. It would be better to use apr_atomic_cas() to set the cleanup bit because we can retry if the refcount changes.
Greg
