You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Daniel Shahaf <da...@elego.de> on 2011/05/19 17:01:15 UTC
Locking and errors (and deserialization) in inprocess-cache
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.
Re: Locking and errors (and deserialization) in inprocess-cache
Posted by 'Daniel Shahaf' <da...@elego.de>.
Bert Huijben wrote on Thu, May 19, 2011 at 17:41:13 +0200:
>
>
> > -----Original Message-----
> > From: Daniel Shahaf [mailto:danielsh@elego.de]
> > Sent: donderdag 19 mei 2011 17:36
> > To: dev@subversion.apache.org
> > Cc: Daniel Shahaf
> > Subject: Re: Locking and errors (and deserialization) in inprocess-cache
> >
> > 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);
>
> But this would crash the application/server if it fails, while the other
> would return an error if a malfunction handler is installed.
>
> So, "no". This is not a valid alternative.
>
> The original assert() before your original patch is a valid alternative as
> that won't crash the application.
>
If we have assert() then a non-debug server would be accessing a cache
whose contents are undefined, which may result in corrupt repositories
(since the cache is used for FS contents). I'd rather crash.
> Bert
>
RE: Locking and errors (and deserialization) in inprocess-cache
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Daniel Shahaf [mailto:danielsh@elego.de]
> Sent: donderdag 19 mei 2011 17:36
> To: dev@subversion.apache.org
> Cc: Daniel Shahaf
> Subject: Re: Locking and errors (and deserialization) in inprocess-cache
>
> 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);
But this would crash the application/server if it fails, while the other
would return an error if a malfunction handler is installed.
So, "no". This is not a valid alternative.
The original assert() before your original patch is a valid alternative as
that won't crash the application.
Bert
Re: Locking and errors (and deserialization) in inprocess-cache
Posted by Daniel Shahaf <da...@elego.de>.
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.