[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: Locking and errors (and deserialization) in inprocess-cache

From: Daniel Shahaf <danielsh_at_elego.de>
Date: Thu, 19 May 2011 17:36:18 +0200

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

This is an archived mail posted to the Subversion Dev mailing list.

This site is subject to the Apache Privacy Policy and the Apache Public Forum Archive Policy.