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.
Received on 2011-05-19 17:02:04 CEST