Actually, the following might suffice for now:
Index: subversion/libsvn_subr/cache-inprocess.c
===================================================================
--- subversion/libsvn_subr/cache-inprocess.c (revision 1124903)
+++ subversion/libsvn_subr/cache-inprocess.c (working copy)
@@ -148,19 +148,19 @@ insert_page(inprocess_cache_t *cache,
/* If PAGE is in the circularly linked list (eg, its NEXT isn't NULL),
* move it to the front of the list. */
-static svn_error_t *
+static void
move_page_to_front(inprocess_cache_t *cache,
struct cache_page *page)
{
- SVN_ERR_ASSERT(page != cache->sentinel);
+ SVN_ERR_ASSERT_NO_RETURN(page != cache->sentinel);
if (! page->next)
- return SVN_NO_ERROR;
+ return;
remove_page_from_list(page);
insert_page(cache, page);
- return SVN_NO_ERROR;
+ return;
}
/* Return a copy of KEY inside POOL, using CACHE->KLEN to figure out
Daniel Shahaf wrote on Thu, May 19, 2011 at 17:01:15 +0200:
> In inprocess-cache.c the following pattern is common:
>
> svn_error_t *inprocess_callback()
> {
> SVN_ERR(lock_cache(cache));
> SVN_ERR(body());
> SVN_ERR(unlock_cache(cache));
> return something;
> }
>
> So, if an error occurs, then all future cache calls deadlock; and it's
> easy to forget to balance lock/unlock calls even by accident.
>
> Suggestions:
>
> * Move to a with_cache_locked() callback paradigm, as already done in
> FSFS and libsvn_wc. This is harder to read but will encourage
> minimizing critical sections and will ensure that the cache is
> properly unlocked on non-fatal error conditions (i.e., those that
> don't correspond to a corrupted cache).
>
> * Alternatively, add SVN_ERR_ASSERT(cache->is_locked) to relevant
> helper functions. This will ensure that locks are either cleared or
> (if stale) noisily complained about, but not deadlock.
>
> * If body() discovers a fatal error condition... well, we could just
> SVN_ERR_ASSERT() out. Or we could set cache->malfunctioning := TRUE
> and then check that at all entry points.
>
> * [ this is somewhat orthogonal ]
>
> The cache passes through (unmodified) all errors from the
> serialize/deserialize functions. Aren't them some conditions that
> we'd like those callbacks to be able to signal (to the cache or to
> the cache's user)? For example:
> SVN_ERR_CACHE_CORRUPT
> SVN_ERR_CACHE_DESERIALIZE_SAID_NOT_FOUND
> SVN_ERR_CACHE_DESERIALIZE_FAILED
>
> Thoughts?
>
> Barring objections I'll look into implementing something that eliminates
> the potential deadlock.
Received on 2011-05-19 17:37:06 CEST