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.
