What about calling memcache_cache_free() instead of the apr_atomic_set(). The refcount must be more than 1, we hold the mutex. It should work just fine.. What do you think?
That looks better as far as not loosing an update from an unlocked thread.
I'm still trying to learn the complete design though. I see that memcache_cache_free() can return with OBJECT_CLEANUP_BIT set, and that decrement_refcount() will react to it and free the memory if it is registered as a cleanup. Will decrement_refcount() always be registered? Just wondering if there are any code paths where we might leak memory.
There's a similar race in remove_entity() using apr_atomic_set() to turn on the cleanup bit:
@@ -536,10 +529,10 @@
* an object marked for cleanup is by design not in the
* hash table.
*/
- if (!obj->cleanup) {
- cache_remove(sconf->cache_cache, obj);
- obj->cleanup = 1;
- ap_log_error(APLOG_MARK, APLOG_INFO, 0, NULL, "gcing a cache entry");
+ if (!(apr_atomic_read(&obj->refcount) & OBJECT_CLEANUP_BIT)) {
+ cache_remove(sconf->cache_cache, obj);
+ apr_atomic_set(&obj->refcount, obj->refcount | OBJECT_CLEANUP_BIT);
+ ap_log_error(APLOG_MARK, APLOG_INFO, 0, NULL, "gcing a cache entry");
}Greg
