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 L. Rall" <dl...@finemaltcoding.com> on 2008/03/12 00:36:22 UTC
Re: svn commit: r29866 - in branches/in-memory-cache/subversion: include libsvn_subr
We should assert() or something if thread_safe is true, but APR_HAS_THREADS
is false.
On Tue, 11 Mar 2008, glasser@tigris.org wrote:
> Author: glasser
> Date: Tue Mar 11 17:11:32 2008
> New Revision: 29866
>
> Log:
> On the in-memory-cache branch:
>
> Support thread_safe argument.
>
> * subversion/include/svn_cache.h
> (svn_cache_create): Document that dup_func can't interact safely
> with the cache.
>
> * subversion/libsvn_subr/cache.c
> (struct svn_cache_t): Add mutex field.
> (svn_cache_create): Create mutex.
> (lock_cache, unlock_cache): New.
> (svn_cache_get, svn_cache_set): Lock and unlock cache.
>
>
> Modified:
> branches/in-memory-cache/subversion/include/svn_cache.h
> branches/in-memory-cache/subversion/libsvn_subr/cache.c
>
> Modified: branches/in-memory-cache/subversion/include/svn_cache.h
> URL: http://svn.collab.net/viewvc/svn/branches/in-memory-cache/subversion/include/svn_cache.h?pathrev=29866&r1=29865&r2=29866
> ==============================================================================
> --- branches/in-memory-cache/subversion/include/svn_cache.h (original)
> +++ branches/in-memory-cache/subversion/include/svn_cache.h Tue Mar 11 17:11:32 2008
> @@ -70,6 +70,8 @@ typedef struct svn_cache_t svn_cache_t;
> *
> * Note that NULL is a legitimate value for cache entries (and @a dup_func
> * will not be called on it).
> + *
> + * It is not safe for @a dup_func to interact with the cache itself.
> */
> svn_error_t *
> svn_cache_create(svn_cache_t **cache_p,
>
> Modified: branches/in-memory-cache/subversion/libsvn_subr/cache.c
> URL: http://svn.collab.net/viewvc/svn/branches/in-memory-cache/subversion/libsvn_subr/cache.c?pathrev=29866&r1=29865&r2=29866
> ==============================================================================
> --- branches/in-memory-cache/subversion/libsvn_subr/cache.c (original)
> +++ branches/in-memory-cache/subversion/libsvn_subr/cache.c Tue Mar 11 17:11:32 2008
> @@ -18,9 +18,13 @@
>
> #include <assert.h>
>
> +#include <apr_thread_mutex.h>
> +
> #include "svn_cache.h"
> #include "svn_pools.h"
>
> +#include "svn_private_config.h"
> +
> /* The cache object. */
> struct svn_cache_t {
> /* Maps from a key (of size CACHE->KLEN) to a struct cache_entry. */
> @@ -55,6 +59,13 @@ struct svn_cache_t {
> * structs, as well as the dup'd values and hash keys.
> */
> apr_pool_t *cache_pool;
> +
> +#if APR_HAS_THREADS
> + /* A lock for intra-process synchronization to the cache, or NULL if
> + * the cache's creator doesn't feel the cache needs to be
> + * thread-safe. */
> + apr_thread_mutex_t *mutex;
> +#endif
> };
>
> /* A cache page; all items on the page are allocated from the same
> @@ -115,7 +126,17 @@ svn_cache_create(svn_cache_t **cache_p,
> /* The sentinel doesn't need a pool. (We're happy to crash if we
> * accidentally try to treat it like a real page.) */
>
> - /* ### TODO: mutex */
> +#if APR_HAS_THREADS
> + if (thread_safe)
> + {
> + apr_status_t status = apr_thread_mutex_create(&(cache->mutex),
> + APR_THREAD_MUTEX_DEFAULT,
> + pool);
> + if (status)
> + return svn_error_wrap_apr(status,
> + _("Can't create cache mutex"));
> + }
> +#endif
>
> cache->cache_pool = pool;
>
> @@ -188,6 +209,41 @@ duplicate_key(svn_cache_t *cache,
> return apr_pmemdup(pool, key, cache->klen);
> }
>
> +/* If applicable, locks CACHE's mutex. */
> +static svn_error_t *
> +lock_cache(svn_cache_t *cache)
> +{
> +#if APR_HAS_THREADS
> + apr_status_t status;
> + if (! cache->mutex)
> + return SVN_NO_ERROR;
> +
> + status = apr_thread_mutex_lock(cache->mutex);
> + if (status)
> + return svn_error_wrap_apr(status, _("Can't lock cache mutex"));
> +#endif
> +
> + return SVN_NO_ERROR;
> +}
> +
> +/* If applicable, unlocks CACHE's mutex, then returns ERR. */
> +static svn_error_t *
> +unlock_cache(svn_cache_t *cache,
> + svn_error_t *err)
> +{
> +#if APR_HAS_THREADS
> + apr_status_t status;
> + if (! cache->mutex)
> + return err;
> +
> + status = apr_thread_mutex_unlock(cache->mutex);
> + if (status && !err)
> + return svn_error_wrap_apr(status, _("Can't unlock cache mutex"));
> +#endif
> +
> + return err;
> +}
> +
> svn_error_t *
> svn_cache_get(void **value_p,
> svn_boolean_t *found,
> @@ -195,25 +251,26 @@ svn_cache_get(void **value_p,
> const void *key,
> apr_pool_t *pool)
> {
> - /* ### TODO: mutex */
> -
> - void *entry_void = apr_hash_get(cache->hash, key, cache->klen);
> + void *entry_void;
> struct cache_entry *entry;
> + svn_error_t *err;
> +
> + SVN_ERR(lock_cache(cache));
>
> + entry_void = apr_hash_get(cache->hash, key, cache->klen);
> if (! entry_void)
> {
> *found = FALSE;
> - return SVN_NO_ERROR;
> + return unlock_cache(cache, SVN_NO_ERROR);
> }
>
> entry = entry_void;
>
> move_page_to_front(cache, entry->page);
>
> - SVN_ERR(duplicate_value(value_p, cache, entry->value, pool));
> *found = TRUE;
> -
> - return SVN_NO_ERROR;
> + err = duplicate_value(value_p, cache, entry->value, pool);
> + return unlock_cache(cache, err);
> }
>
> /* Removes PAGE from the LRU list, removes all of its entries from
> @@ -252,7 +309,12 @@ svn_cache_set(svn_cache_t *cache,
> void *value,
> apr_pool_t *pool)
> {
> - void *existing_entry = apr_hash_get(cache->hash, key, cache->klen);
> + void *existing_entry;
> + svn_error_t *err = SVN_NO_ERROR;
> +
> + SVN_ERR(lock_cache(cache));
> +
> + existing_entry = apr_hash_get(cache->hash, key, cache->klen);
>
> /* Is it already here, but we can do the one-item-per-page
> * optimization? */
> @@ -281,8 +343,8 @@ svn_cache_set(svn_cache_t *cache,
> struct cache_page *page = entry->page;
>
> move_page_to_front(cache, page);
> - SVN_ERR(duplicate_value(&(entry->value), cache, value, page->page_pool));
> - return SVN_NO_ERROR;
> + err = duplicate_value(&(entry->value), cache, value, page->page_pool);
> + goto cleanup;
> }
>
> /* Do we not have a partial page to put it on, but we are allowed to
> @@ -318,8 +380,10 @@ svn_cache_set(svn_cache_t *cache,
>
> /* Copy the key and value into the page's pool. */
> new_entry->key = duplicate_key(cache, key, page->page_pool);
> - SVN_ERR(duplicate_value(&(new_entry->value), cache, value,
> - page->page_pool));
> + err = duplicate_value(&(new_entry->value), cache, value,
> + page->page_pool);
> + if (err)
> + goto cleanup;
>
> /* Add the entry to the page's list. */
> new_entry->page = page;
> @@ -341,6 +405,6 @@ svn_cache_set(svn_cache_t *cache,
> }
> }
>
> - /* ### TODO: mutex */
> - return SVN_NO_ERROR;
> + cleanup:
> + return unlock_cache(cache, err);
> }
Re: svn commit: r29866 - in branches/in-memory-cache/subversion: include libsvn_subr
Posted by "Daniel L. Rall" <dl...@finemaltcoding.com>.
Maybe with Apache httpd using the pre-fork MPM? Not sure.
Regarding "fixing it", is it possible to store the mutex in the shared memory
segment in a manner which renders it still usable? In any case, I was just
suggesting punting on this one in a way which provides some user feedback. :-)
On Tue, 11 Mar 2008, David Glasser wrote:
> Huh. Does anything like this ever happen in our Subversion code? (In
> any case, it's not like a thread mutex is going to fix that, no?)
>
> --dave
>
> On Tue, Mar 11, 2008 at 7:08 PM, Daniel L. Rall <dl...@finemaltcoding.com> wrote:
> > APR_HASH_THREADS=false doesn't mean that multiple processes can't access
> > the cache; consider the situation where an mmap()'d memory segment is shared
> > by multiple processes accessing the same cache. I'm not adverse to ignoring
> > this edge case in the code, so long as the documentation is clear.
> >
> >
> >
> > On Tue, 11 Mar 2008, David Glasser wrote:
> >
> > > Nah. "thread_safe = TRUE" means "use mutexes if necessary";
> > > "thread_safe = FALSE" means "this object will always be kept within
> > > one thread". It's a function of whether or not the cache could
> > > theoretically be shared between threads. If APR doesn't have threads,
> > > then there's n problem. I don't think there's any need to have the
> > > ugliness of an API whose argument count depends on the existence of
> > > APR_HAS_THREADS.
> > >
> > > --dave
> > >
> > > On Tue, Mar 11, 2008 at 5:36 PM, Daniel L. Rall <dl...@finemaltcoding.com> wrote:
> > > > We should assert() or something if thread_safe is true, but APR_HAS_THREADS
> > > > is false.
> > > >
> > > >
> > > >
> > > > On Tue, 11 Mar 2008, glasser@tigris.org wrote:
> > > >
> > > > > Author: glasser
> > > > > Date: Tue Mar 11 17:11:32 2008
> > > > > New Revision: 29866
> > > > >
> > > > > Log:
> > > > > On the in-memory-cache branch:
> > > > >
> > > > > Support thread_safe argument.
> > > > >
> > > > > * subversion/include/svn_cache.h
> > > > > (svn_cache_create): Document that dup_func can't interact safely
> > > > > with the cache.
> > > > >
> > > > > * subversion/libsvn_subr/cache.c
> > > > > (struct svn_cache_t): Add mutex field.
> > > > > (svn_cache_create): Create mutex.
> > > > > (lock_cache, unlock_cache): New.
> > > > > (svn_cache_get, svn_cache_set): Lock and unlock cache.
> > > > >
> > > > >
> > > > > Modified:
> > > > > branches/in-memory-cache/subversion/include/svn_cache.h
> > > > > branches/in-memory-cache/subversion/libsvn_subr/cache.c
> > > > >
> > > > > Modified: branches/in-memory-cache/subversion/include/svn_cache.h
> > > > > URL: http://svn.collab.net/viewvc/svn/branches/in-memory-cache/subversion/include/svn_cache.h?pathrev=29866&r1=29865&r2=29866
> > > > > ==============================================================================
> > > > > --- branches/in-memory-cache/subversion/include/svn_cache.h (original)
> > > > > +++ branches/in-memory-cache/subversion/include/svn_cache.h Tue Mar 11 17:11:32 2008
> > > > > @@ -70,6 +70,8 @@ typedef struct svn_cache_t svn_cache_t;
> > > > > *
> > > > > * Note that NULL is a legitimate value for cache entries (and @a dup_func
> > > > > * will not be called on it).
> > > > > + *
> > > > > + * It is not safe for @a dup_func to interact with the cache itself.
> > > > > */
> > > > > svn_error_t *
> > > > > svn_cache_create(svn_cache_t **cache_p,
> > > > >
> > > > > Modified: branches/in-memory-cache/subversion/libsvn_subr/cache.c
> > > > > URL: http://svn.collab.net/viewvc/svn/branches/in-memory-cache/subversion/libsvn_subr/cache.c?pathrev=29866&r1=29865&r2=29866
> > > > > ==============================================================================
> > > > > --- branches/in-memory-cache/subversion/libsvn_subr/cache.c (original)
> > > > > +++ branches/in-memory-cache/subversion/libsvn_subr/cache.c Tue Mar 11 17:11:32 2008
> > > > > @@ -18,9 +18,13 @@
> > > > >
> > > > > #include <assert.h>
> > > > >
> > > > > +#include <apr_thread_mutex.h>
> > > > > +
> > > > > #include "svn_cache.h"
> > > > > #include "svn_pools.h"
> > > > >
> > > > > +#include "svn_private_config.h"
> > > > > +
> > > > > /* The cache object. */
> > > > > struct svn_cache_t {
> > > > > /* Maps from a key (of size CACHE->KLEN) to a struct cache_entry. */
> > > > > @@ -55,6 +59,13 @@ struct svn_cache_t {
> > > > > * structs, as well as the dup'd values and hash keys.
> > > > > */
> > > > > apr_pool_t *cache_pool;
> > > > > +
> > > > > +#if APR_HAS_THREADS
> > > > > + /* A lock for intra-process synchronization to the cache, or NULL if
> > > > > + * the cache's creator doesn't feel the cache needs to be
> > > > > + * thread-safe. */
> > > > > + apr_thread_mutex_t *mutex;
> > > > > +#endif
> > > > > };
> > > > >
> > > > > /* A cache page; all items on the page are allocated from the same
> > > > > @@ -115,7 +126,17 @@ svn_cache_create(svn_cache_t **cache_p,
> > > > > /* The sentinel doesn't need a pool. (We're happy to crash if we
> > > > > * accidentally try to treat it like a real page.) */
> > > > >
> > > > > - /* ### TODO: mutex */
> > > > > +#if APR_HAS_THREADS
> > > > > + if (thread_safe)
> > > > > + {
> > > > > + apr_status_t status = apr_thread_mutex_create(&(cache->mutex),
> > > > > + APR_THREAD_MUTEX_DEFAULT,
> > > > > + pool);
> > > > > + if (status)
> > > > > + return svn_error_wrap_apr(status,
> > > > > + _("Can't create cache mutex"));
> > > > > + }
> > > > > +#endif
> > > > >
> > > > > cache->cache_pool = pool;
> > > > >
> > > > > @@ -188,6 +209,41 @@ duplicate_key(svn_cache_t *cache,
> > > > > return apr_pmemdup(pool, key, cache->klen);
> > > > > }
> > > > >
> > > > > +/* If applicable, locks CACHE's mutex. */
> > > > > +static svn_error_t *
> > > > > +lock_cache(svn_cache_t *cache)
> > > > > +{
> > > > > +#if APR_HAS_THREADS
> > > > > + apr_status_t status;
> > > > > + if (! cache->mutex)
> > > > > + return SVN_NO_ERROR;
> > > > > +
> > > > > + status = apr_thread_mutex_lock(cache->mutex);
> > > > > + if (status)
> > > > > + return svn_error_wrap_apr(status, _("Can't lock cache mutex"));
> > > > > +#endif
> > > > > +
> > > > > + return SVN_NO_ERROR;
> > > > > +}
> > > > > +
> > > > > +/* If applicable, unlocks CACHE's mutex, then returns ERR. */
> > > > > +static svn_error_t *
> > > > > +unlock_cache(svn_cache_t *cache,
> > > > > + svn_error_t *err)
> > > > > +{
> > > > > +#if APR_HAS_THREADS
> > > > > + apr_status_t status;
> > > > > + if (! cache->mutex)
> > > > > + return err;
> > > > > +
> > > > > + status = apr_thread_mutex_unlock(cache->mutex);
> > > > > + if (status && !err)
> > > > > + return svn_error_wrap_apr(status, _("Can't unlock cache mutex"));
> > > > > +#endif
> > > > > +
> > > > > + return err;
> > > > > +}
> > > > > +
> > > > > svn_error_t *
> > > > > svn_cache_get(void **value_p,
> > > > > svn_boolean_t *found,
> > > > > @@ -195,25 +251,26 @@ svn_cache_get(void **value_p,
> > > > > const void *key,
> > > > > apr_pool_t *pool)
> > > > > {
> > > > > - /* ### TODO: mutex */
> > > > > -
> > > > > - void *entry_void = apr_hash_get(cache->hash, key, cache->klen);
> > > > > + void *entry_void;
> > > > > struct cache_entry *entry;
> > > > > + svn_error_t *err;
> > > > > +
> > > > > + SVN_ERR(lock_cache(cache));
> > > > >
> > > > > + entry_void = apr_hash_get(cache->hash, key, cache->klen);
> > > > > if (! entry_void)
> > > > > {
> > > > > *found = FALSE;
> > > > > - return SVN_NO_ERROR;
> > > > > + return unlock_cache(cache, SVN_NO_ERROR);
> > > > > }
> > > > >
> > > > > entry = entry_void;
> > > > >
> > > > > move_page_to_front(cache, entry->page);
> > > > >
> > > > > - SVN_ERR(duplicate_value(value_p, cache, entry->value, pool));
> > > > > *found = TRUE;
> > > > > -
> > > > > - return SVN_NO_ERROR;
> > > > > + err = duplicate_value(value_p, cache, entry->value, pool);
> > > > > + return unlock_cache(cache, err);
> > > > > }
> > > > >
> > > > > /* Removes PAGE from the LRU list, removes all of its entries from
> > > > > @@ -252,7 +309,12 @@ svn_cache_set(svn_cache_t *cache,
> > > > > void *value,
> > > > > apr_pool_t *pool)
> > > > > {
> > > > > - void *existing_entry = apr_hash_get(cache->hash, key, cache->klen);
> > > > > + void *existing_entry;
> > > > > + svn_error_t *err = SVN_NO_ERROR;
> > > > > +
> > > > > + SVN_ERR(lock_cache(cache));
> > > > > +
> > > > > + existing_entry = apr_hash_get(cache->hash, key, cache->klen);
> > > > >
> > > > > /* Is it already here, but we can do the one-item-per-page
> > > > > * optimization? */
> > > > > @@ -281,8 +343,8 @@ svn_cache_set(svn_cache_t *cache,
> > > > > struct cache_page *page = entry->page;
> > > > >
> > > > > move_page_to_front(cache, page);
> > > > > - SVN_ERR(duplicate_value(&(entry->value), cache, value, page->page_pool));
> > > > > - return SVN_NO_ERROR;
> > > > > + err = duplicate_value(&(entry->value), cache, value, page->page_pool);
> > > > > + goto cleanup;
> > > > > }
> > > > >
> > > > > /* Do we not have a partial page to put it on, but we are allowed to
> > > > > @@ -318,8 +380,10 @@ svn_cache_set(svn_cache_t *cache,
> > > > >
> > > > > /* Copy the key and value into the page's pool. */
> > > > > new_entry->key = duplicate_key(cache, key, page->page_pool);
> > > > > - SVN_ERR(duplicate_value(&(new_entry->value), cache, value,
> > > > > - page->page_pool));
> > > > > + err = duplicate_value(&(new_entry->value), cache, value,
> > > > > + page->page_pool);
> > > > > + if (err)
> > > > > + goto cleanup;
> > > > >
> > > > > /* Add the entry to the page's list. */
> > > > > new_entry->page = page;
> > > > > @@ -341,6 +405,6 @@ svn_cache_set(svn_cache_t *cache,
> > > > > }
> > > > > }
> > > > >
> > > > > - /* ### TODO: mutex */
> > > > > - return SVN_NO_ERROR;
> > > > > + cleanup:
> > > > > + return unlock_cache(cache, err);
> > > > > }
> > > >
> > >
> > >
> > >
> > > --
> > > David Glasser | glasser@davidglasser.net | http://www.davidglasser.net/
> >
> > --
> >
> > Daniel Rall
> >
>
>
>
> --
> David Glasser | glasser@davidglasser.net | http://www.davidglasser.net/
--
Daniel Rall
Re: svn commit: r29866 - in branches/in-memory-cache/subversion: include libsvn_subr
Posted by David Glasser <gl...@davidglasser.net>.
Huh. Does anything like this ever happen in our Subversion code? (In
any case, it's not like a thread mutex is going to fix that, no?)
--dave
On Tue, Mar 11, 2008 at 7:08 PM, Daniel L. Rall <dl...@finemaltcoding.com> wrote:
> APR_HASH_THREADS=false doesn't mean that multiple processes can't access
> the cache; consider the situation where an mmap()'d memory segment is shared
> by multiple processes accessing the same cache. I'm not adverse to ignoring
> this edge case in the code, so long as the documentation is clear.
>
>
>
> On Tue, 11 Mar 2008, David Glasser wrote:
>
> > Nah. "thread_safe = TRUE" means "use mutexes if necessary";
> > "thread_safe = FALSE" means "this object will always be kept within
> > one thread". It's a function of whether or not the cache could
> > theoretically be shared between threads. If APR doesn't have threads,
> > then there's n problem. I don't think there's any need to have the
> > ugliness of an API whose argument count depends on the existence of
> > APR_HAS_THREADS.
> >
> > --dave
> >
> > On Tue, Mar 11, 2008 at 5:36 PM, Daniel L. Rall <dl...@finemaltcoding.com> wrote:
> > > We should assert() or something if thread_safe is true, but APR_HAS_THREADS
> > > is false.
> > >
> > >
> > >
> > > On Tue, 11 Mar 2008, glasser@tigris.org wrote:
> > >
> > > > Author: glasser
> > > > Date: Tue Mar 11 17:11:32 2008
> > > > New Revision: 29866
> > > >
> > > > Log:
> > > > On the in-memory-cache branch:
> > > >
> > > > Support thread_safe argument.
> > > >
> > > > * subversion/include/svn_cache.h
> > > > (svn_cache_create): Document that dup_func can't interact safely
> > > > with the cache.
> > > >
> > > > * subversion/libsvn_subr/cache.c
> > > > (struct svn_cache_t): Add mutex field.
> > > > (svn_cache_create): Create mutex.
> > > > (lock_cache, unlock_cache): New.
> > > > (svn_cache_get, svn_cache_set): Lock and unlock cache.
> > > >
> > > >
> > > > Modified:
> > > > branches/in-memory-cache/subversion/include/svn_cache.h
> > > > branches/in-memory-cache/subversion/libsvn_subr/cache.c
> > > >
> > > > Modified: branches/in-memory-cache/subversion/include/svn_cache.h
> > > > URL: http://svn.collab.net/viewvc/svn/branches/in-memory-cache/subversion/include/svn_cache.h?pathrev=29866&r1=29865&r2=29866
> > > > ==============================================================================
> > > > --- branches/in-memory-cache/subversion/include/svn_cache.h (original)
> > > > +++ branches/in-memory-cache/subversion/include/svn_cache.h Tue Mar 11 17:11:32 2008
> > > > @@ -70,6 +70,8 @@ typedef struct svn_cache_t svn_cache_t;
> > > > *
> > > > * Note that NULL is a legitimate value for cache entries (and @a dup_func
> > > > * will not be called on it).
> > > > + *
> > > > + * It is not safe for @a dup_func to interact with the cache itself.
> > > > */
> > > > svn_error_t *
> > > > svn_cache_create(svn_cache_t **cache_p,
> > > >
> > > > Modified: branches/in-memory-cache/subversion/libsvn_subr/cache.c
> > > > URL: http://svn.collab.net/viewvc/svn/branches/in-memory-cache/subversion/libsvn_subr/cache.c?pathrev=29866&r1=29865&r2=29866
> > > > ==============================================================================
> > > > --- branches/in-memory-cache/subversion/libsvn_subr/cache.c (original)
> > > > +++ branches/in-memory-cache/subversion/libsvn_subr/cache.c Tue Mar 11 17:11:32 2008
> > > > @@ -18,9 +18,13 @@
> > > >
> > > > #include <assert.h>
> > > >
> > > > +#include <apr_thread_mutex.h>
> > > > +
> > > > #include "svn_cache.h"
> > > > #include "svn_pools.h"
> > > >
> > > > +#include "svn_private_config.h"
> > > > +
> > > > /* The cache object. */
> > > > struct svn_cache_t {
> > > > /* Maps from a key (of size CACHE->KLEN) to a struct cache_entry. */
> > > > @@ -55,6 +59,13 @@ struct svn_cache_t {
> > > > * structs, as well as the dup'd values and hash keys.
> > > > */
> > > > apr_pool_t *cache_pool;
> > > > +
> > > > +#if APR_HAS_THREADS
> > > > + /* A lock for intra-process synchronization to the cache, or NULL if
> > > > + * the cache's creator doesn't feel the cache needs to be
> > > > + * thread-safe. */
> > > > + apr_thread_mutex_t *mutex;
> > > > +#endif
> > > > };
> > > >
> > > > /* A cache page; all items on the page are allocated from the same
> > > > @@ -115,7 +126,17 @@ svn_cache_create(svn_cache_t **cache_p,
> > > > /* The sentinel doesn't need a pool. (We're happy to crash if we
> > > > * accidentally try to treat it like a real page.) */
> > > >
> > > > - /* ### TODO: mutex */
> > > > +#if APR_HAS_THREADS
> > > > + if (thread_safe)
> > > > + {
> > > > + apr_status_t status = apr_thread_mutex_create(&(cache->mutex),
> > > > + APR_THREAD_MUTEX_DEFAULT,
> > > > + pool);
> > > > + if (status)
> > > > + return svn_error_wrap_apr(status,
> > > > + _("Can't create cache mutex"));
> > > > + }
> > > > +#endif
> > > >
> > > > cache->cache_pool = pool;
> > > >
> > > > @@ -188,6 +209,41 @@ duplicate_key(svn_cache_t *cache,
> > > > return apr_pmemdup(pool, key, cache->klen);
> > > > }
> > > >
> > > > +/* If applicable, locks CACHE's mutex. */
> > > > +static svn_error_t *
> > > > +lock_cache(svn_cache_t *cache)
> > > > +{
> > > > +#if APR_HAS_THREADS
> > > > + apr_status_t status;
> > > > + if (! cache->mutex)
> > > > + return SVN_NO_ERROR;
> > > > +
> > > > + status = apr_thread_mutex_lock(cache->mutex);
> > > > + if (status)
> > > > + return svn_error_wrap_apr(status, _("Can't lock cache mutex"));
> > > > +#endif
> > > > +
> > > > + return SVN_NO_ERROR;
> > > > +}
> > > > +
> > > > +/* If applicable, unlocks CACHE's mutex, then returns ERR. */
> > > > +static svn_error_t *
> > > > +unlock_cache(svn_cache_t *cache,
> > > > + svn_error_t *err)
> > > > +{
> > > > +#if APR_HAS_THREADS
> > > > + apr_status_t status;
> > > > + if (! cache->mutex)
> > > > + return err;
> > > > +
> > > > + status = apr_thread_mutex_unlock(cache->mutex);
> > > > + if (status && !err)
> > > > + return svn_error_wrap_apr(status, _("Can't unlock cache mutex"));
> > > > +#endif
> > > > +
> > > > + return err;
> > > > +}
> > > > +
> > > > svn_error_t *
> > > > svn_cache_get(void **value_p,
> > > > svn_boolean_t *found,
> > > > @@ -195,25 +251,26 @@ svn_cache_get(void **value_p,
> > > > const void *key,
> > > > apr_pool_t *pool)
> > > > {
> > > > - /* ### TODO: mutex */
> > > > -
> > > > - void *entry_void = apr_hash_get(cache->hash, key, cache->klen);
> > > > + void *entry_void;
> > > > struct cache_entry *entry;
> > > > + svn_error_t *err;
> > > > +
> > > > + SVN_ERR(lock_cache(cache));
> > > >
> > > > + entry_void = apr_hash_get(cache->hash, key, cache->klen);
> > > > if (! entry_void)
> > > > {
> > > > *found = FALSE;
> > > > - return SVN_NO_ERROR;
> > > > + return unlock_cache(cache, SVN_NO_ERROR);
> > > > }
> > > >
> > > > entry = entry_void;
> > > >
> > > > move_page_to_front(cache, entry->page);
> > > >
> > > > - SVN_ERR(duplicate_value(value_p, cache, entry->value, pool));
> > > > *found = TRUE;
> > > > -
> > > > - return SVN_NO_ERROR;
> > > > + err = duplicate_value(value_p, cache, entry->value, pool);
> > > > + return unlock_cache(cache, err);
> > > > }
> > > >
> > > > /* Removes PAGE from the LRU list, removes all of its entries from
> > > > @@ -252,7 +309,12 @@ svn_cache_set(svn_cache_t *cache,
> > > > void *value,
> > > > apr_pool_t *pool)
> > > > {
> > > > - void *existing_entry = apr_hash_get(cache->hash, key, cache->klen);
> > > > + void *existing_entry;
> > > > + svn_error_t *err = SVN_NO_ERROR;
> > > > +
> > > > + SVN_ERR(lock_cache(cache));
> > > > +
> > > > + existing_entry = apr_hash_get(cache->hash, key, cache->klen);
> > > >
> > > > /* Is it already here, but we can do the one-item-per-page
> > > > * optimization? */
> > > > @@ -281,8 +343,8 @@ svn_cache_set(svn_cache_t *cache,
> > > > struct cache_page *page = entry->page;
> > > >
> > > > move_page_to_front(cache, page);
> > > > - SVN_ERR(duplicate_value(&(entry->value), cache, value, page->page_pool));
> > > > - return SVN_NO_ERROR;
> > > > + err = duplicate_value(&(entry->value), cache, value, page->page_pool);
> > > > + goto cleanup;
> > > > }
> > > >
> > > > /* Do we not have a partial page to put it on, but we are allowed to
> > > > @@ -318,8 +380,10 @@ svn_cache_set(svn_cache_t *cache,
> > > >
> > > > /* Copy the key and value into the page's pool. */
> > > > new_entry->key = duplicate_key(cache, key, page->page_pool);
> > > > - SVN_ERR(duplicate_value(&(new_entry->value), cache, value,
> > > > - page->page_pool));
> > > > + err = duplicate_value(&(new_entry->value), cache, value,
> > > > + page->page_pool);
> > > > + if (err)
> > > > + goto cleanup;
> > > >
> > > > /* Add the entry to the page's list. */
> > > > new_entry->page = page;
> > > > @@ -341,6 +405,6 @@ svn_cache_set(svn_cache_t *cache,
> > > > }
> > > > }
> > > >
> > > > - /* ### TODO: mutex */
> > > > - return SVN_NO_ERROR;
> > > > + cleanup:
> > > > + return unlock_cache(cache, err);
> > > > }
> > >
> >
> >
> >
> > --
> > David Glasser | glasser@davidglasser.net | http://www.davidglasser.net/
>
> --
>
> Daniel Rall
>
--
David Glasser | glasser@davidglasser.net | http://www.davidglasser.net/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r29866 - in branches/in-memory-cache/subversion: include libsvn_subr
Posted by "Daniel L. Rall" <dl...@finemaltcoding.com>.
APR_HASH_THREADS=false doesn't mean that multiple processes can't access
the cache; consider the situation where an mmap()'d memory segment is shared
by multiple processes accessing the same cache. I'm not adverse to ignoring
this edge case in the code, so long as the documentation is clear.
On Tue, 11 Mar 2008, David Glasser wrote:
> Nah. "thread_safe = TRUE" means "use mutexes if necessary";
> "thread_safe = FALSE" means "this object will always be kept within
> one thread". It's a function of whether or not the cache could
> theoretically be shared between threads. If APR doesn't have threads,
> then there's n problem. I don't think there's any need to have the
> ugliness of an API whose argument count depends on the existence of
> APR_HAS_THREADS.
>
> --dave
>
> On Tue, Mar 11, 2008 at 5:36 PM, Daniel L. Rall <dl...@finemaltcoding.com> wrote:
> > We should assert() or something if thread_safe is true, but APR_HAS_THREADS
> > is false.
> >
> >
> >
> > On Tue, 11 Mar 2008, glasser@tigris.org wrote:
> >
> > > Author: glasser
> > > Date: Tue Mar 11 17:11:32 2008
> > > New Revision: 29866
> > >
> > > Log:
> > > On the in-memory-cache branch:
> > >
> > > Support thread_safe argument.
> > >
> > > * subversion/include/svn_cache.h
> > > (svn_cache_create): Document that dup_func can't interact safely
> > > with the cache.
> > >
> > > * subversion/libsvn_subr/cache.c
> > > (struct svn_cache_t): Add mutex field.
> > > (svn_cache_create): Create mutex.
> > > (lock_cache, unlock_cache): New.
> > > (svn_cache_get, svn_cache_set): Lock and unlock cache.
> > >
> > >
> > > Modified:
> > > branches/in-memory-cache/subversion/include/svn_cache.h
> > > branches/in-memory-cache/subversion/libsvn_subr/cache.c
> > >
> > > Modified: branches/in-memory-cache/subversion/include/svn_cache.h
> > > URL: http://svn.collab.net/viewvc/svn/branches/in-memory-cache/subversion/include/svn_cache.h?pathrev=29866&r1=29865&r2=29866
> > > ==============================================================================
> > > --- branches/in-memory-cache/subversion/include/svn_cache.h (original)
> > > +++ branches/in-memory-cache/subversion/include/svn_cache.h Tue Mar 11 17:11:32 2008
> > > @@ -70,6 +70,8 @@ typedef struct svn_cache_t svn_cache_t;
> > > *
> > > * Note that NULL is a legitimate value for cache entries (and @a dup_func
> > > * will not be called on it).
> > > + *
> > > + * It is not safe for @a dup_func to interact with the cache itself.
> > > */
> > > svn_error_t *
> > > svn_cache_create(svn_cache_t **cache_p,
> > >
> > > Modified: branches/in-memory-cache/subversion/libsvn_subr/cache.c
> > > URL: http://svn.collab.net/viewvc/svn/branches/in-memory-cache/subversion/libsvn_subr/cache.c?pathrev=29866&r1=29865&r2=29866
> > > ==============================================================================
> > > --- branches/in-memory-cache/subversion/libsvn_subr/cache.c (original)
> > > +++ branches/in-memory-cache/subversion/libsvn_subr/cache.c Tue Mar 11 17:11:32 2008
> > > @@ -18,9 +18,13 @@
> > >
> > > #include <assert.h>
> > >
> > > +#include <apr_thread_mutex.h>
> > > +
> > > #include "svn_cache.h"
> > > #include "svn_pools.h"
> > >
> > > +#include "svn_private_config.h"
> > > +
> > > /* The cache object. */
> > > struct svn_cache_t {
> > > /* Maps from a key (of size CACHE->KLEN) to a struct cache_entry. */
> > > @@ -55,6 +59,13 @@ struct svn_cache_t {
> > > * structs, as well as the dup'd values and hash keys.
> > > */
> > > apr_pool_t *cache_pool;
> > > +
> > > +#if APR_HAS_THREADS
> > > + /* A lock for intra-process synchronization to the cache, or NULL if
> > > + * the cache's creator doesn't feel the cache needs to be
> > > + * thread-safe. */
> > > + apr_thread_mutex_t *mutex;
> > > +#endif
> > > };
> > >
> > > /* A cache page; all items on the page are allocated from the same
> > > @@ -115,7 +126,17 @@ svn_cache_create(svn_cache_t **cache_p,
> > > /* The sentinel doesn't need a pool. (We're happy to crash if we
> > > * accidentally try to treat it like a real page.) */
> > >
> > > - /* ### TODO: mutex */
> > > +#if APR_HAS_THREADS
> > > + if (thread_safe)
> > > + {
> > > + apr_status_t status = apr_thread_mutex_create(&(cache->mutex),
> > > + APR_THREAD_MUTEX_DEFAULT,
> > > + pool);
> > > + if (status)
> > > + return svn_error_wrap_apr(status,
> > > + _("Can't create cache mutex"));
> > > + }
> > > +#endif
> > >
> > > cache->cache_pool = pool;
> > >
> > > @@ -188,6 +209,41 @@ duplicate_key(svn_cache_t *cache,
> > > return apr_pmemdup(pool, key, cache->klen);
> > > }
> > >
> > > +/* If applicable, locks CACHE's mutex. */
> > > +static svn_error_t *
> > > +lock_cache(svn_cache_t *cache)
> > > +{
> > > +#if APR_HAS_THREADS
> > > + apr_status_t status;
> > > + if (! cache->mutex)
> > > + return SVN_NO_ERROR;
> > > +
> > > + status = apr_thread_mutex_lock(cache->mutex);
> > > + if (status)
> > > + return svn_error_wrap_apr(status, _("Can't lock cache mutex"));
> > > +#endif
> > > +
> > > + return SVN_NO_ERROR;
> > > +}
> > > +
> > > +/* If applicable, unlocks CACHE's mutex, then returns ERR. */
> > > +static svn_error_t *
> > > +unlock_cache(svn_cache_t *cache,
> > > + svn_error_t *err)
> > > +{
> > > +#if APR_HAS_THREADS
> > > + apr_status_t status;
> > > + if (! cache->mutex)
> > > + return err;
> > > +
> > > + status = apr_thread_mutex_unlock(cache->mutex);
> > > + if (status && !err)
> > > + return svn_error_wrap_apr(status, _("Can't unlock cache mutex"));
> > > +#endif
> > > +
> > > + return err;
> > > +}
> > > +
> > > svn_error_t *
> > > svn_cache_get(void **value_p,
> > > svn_boolean_t *found,
> > > @@ -195,25 +251,26 @@ svn_cache_get(void **value_p,
> > > const void *key,
> > > apr_pool_t *pool)
> > > {
> > > - /* ### TODO: mutex */
> > > -
> > > - void *entry_void = apr_hash_get(cache->hash, key, cache->klen);
> > > + void *entry_void;
> > > struct cache_entry *entry;
> > > + svn_error_t *err;
> > > +
> > > + SVN_ERR(lock_cache(cache));
> > >
> > > + entry_void = apr_hash_get(cache->hash, key, cache->klen);
> > > if (! entry_void)
> > > {
> > > *found = FALSE;
> > > - return SVN_NO_ERROR;
> > > + return unlock_cache(cache, SVN_NO_ERROR);
> > > }
> > >
> > > entry = entry_void;
> > >
> > > move_page_to_front(cache, entry->page);
> > >
> > > - SVN_ERR(duplicate_value(value_p, cache, entry->value, pool));
> > > *found = TRUE;
> > > -
> > > - return SVN_NO_ERROR;
> > > + err = duplicate_value(value_p, cache, entry->value, pool);
> > > + return unlock_cache(cache, err);
> > > }
> > >
> > > /* Removes PAGE from the LRU list, removes all of its entries from
> > > @@ -252,7 +309,12 @@ svn_cache_set(svn_cache_t *cache,
> > > void *value,
> > > apr_pool_t *pool)
> > > {
> > > - void *existing_entry = apr_hash_get(cache->hash, key, cache->klen);
> > > + void *existing_entry;
> > > + svn_error_t *err = SVN_NO_ERROR;
> > > +
> > > + SVN_ERR(lock_cache(cache));
> > > +
> > > + existing_entry = apr_hash_get(cache->hash, key, cache->klen);
> > >
> > > /* Is it already here, but we can do the one-item-per-page
> > > * optimization? */
> > > @@ -281,8 +343,8 @@ svn_cache_set(svn_cache_t *cache,
> > > struct cache_page *page = entry->page;
> > >
> > > move_page_to_front(cache, page);
> > > - SVN_ERR(duplicate_value(&(entry->value), cache, value, page->page_pool));
> > > - return SVN_NO_ERROR;
> > > + err = duplicate_value(&(entry->value), cache, value, page->page_pool);
> > > + goto cleanup;
> > > }
> > >
> > > /* Do we not have a partial page to put it on, but we are allowed to
> > > @@ -318,8 +380,10 @@ svn_cache_set(svn_cache_t *cache,
> > >
> > > /* Copy the key and value into the page's pool. */
> > > new_entry->key = duplicate_key(cache, key, page->page_pool);
> > > - SVN_ERR(duplicate_value(&(new_entry->value), cache, value,
> > > - page->page_pool));
> > > + err = duplicate_value(&(new_entry->value), cache, value,
> > > + page->page_pool);
> > > + if (err)
> > > + goto cleanup;
> > >
> > > /* Add the entry to the page's list. */
> > > new_entry->page = page;
> > > @@ -341,6 +405,6 @@ svn_cache_set(svn_cache_t *cache,
> > > }
> > > }
> > >
> > > - /* ### TODO: mutex */
> > > - return SVN_NO_ERROR;
> > > + cleanup:
> > > + return unlock_cache(cache, err);
> > > }
> >
>
>
>
> --
> David Glasser | glasser@davidglasser.net | http://www.davidglasser.net/
--
Daniel Rall
Re: svn commit: r29866 - in branches/in-memory-cache/subversion: include libsvn_subr
Posted by David Glasser <gl...@davidglasser.net>.
Nah. "thread_safe = TRUE" means "use mutexes if necessary";
"thread_safe = FALSE" means "this object will always be kept within
one thread". It's a function of whether or not the cache could
theoretically be shared between threads. If APR doesn't have threads,
then there's n problem. I don't think there's any need to have the
ugliness of an API whose argument count depends on the existence of
APR_HAS_THREADS.
--dave
On Tue, Mar 11, 2008 at 5:36 PM, Daniel L. Rall <dl...@finemaltcoding.com> wrote:
> We should assert() or something if thread_safe is true, but APR_HAS_THREADS
> is false.
>
>
>
> On Tue, 11 Mar 2008, glasser@tigris.org wrote:
>
> > Author: glasser
> > Date: Tue Mar 11 17:11:32 2008
> > New Revision: 29866
> >
> > Log:
> > On the in-memory-cache branch:
> >
> > Support thread_safe argument.
> >
> > * subversion/include/svn_cache.h
> > (svn_cache_create): Document that dup_func can't interact safely
> > with the cache.
> >
> > * subversion/libsvn_subr/cache.c
> > (struct svn_cache_t): Add mutex field.
> > (svn_cache_create): Create mutex.
> > (lock_cache, unlock_cache): New.
> > (svn_cache_get, svn_cache_set): Lock and unlock cache.
> >
> >
> > Modified:
> > branches/in-memory-cache/subversion/include/svn_cache.h
> > branches/in-memory-cache/subversion/libsvn_subr/cache.c
> >
> > Modified: branches/in-memory-cache/subversion/include/svn_cache.h
> > URL: http://svn.collab.net/viewvc/svn/branches/in-memory-cache/subversion/include/svn_cache.h?pathrev=29866&r1=29865&r2=29866
> > ==============================================================================
> > --- branches/in-memory-cache/subversion/include/svn_cache.h (original)
> > +++ branches/in-memory-cache/subversion/include/svn_cache.h Tue Mar 11 17:11:32 2008
> > @@ -70,6 +70,8 @@ typedef struct svn_cache_t svn_cache_t;
> > *
> > * Note that NULL is a legitimate value for cache entries (and @a dup_func
> > * will not be called on it).
> > + *
> > + * It is not safe for @a dup_func to interact with the cache itself.
> > */
> > svn_error_t *
> > svn_cache_create(svn_cache_t **cache_p,
> >
> > Modified: branches/in-memory-cache/subversion/libsvn_subr/cache.c
> > URL: http://svn.collab.net/viewvc/svn/branches/in-memory-cache/subversion/libsvn_subr/cache.c?pathrev=29866&r1=29865&r2=29866
> > ==============================================================================
> > --- branches/in-memory-cache/subversion/libsvn_subr/cache.c (original)
> > +++ branches/in-memory-cache/subversion/libsvn_subr/cache.c Tue Mar 11 17:11:32 2008
> > @@ -18,9 +18,13 @@
> >
> > #include <assert.h>
> >
> > +#include <apr_thread_mutex.h>
> > +
> > #include "svn_cache.h"
> > #include "svn_pools.h"
> >
> > +#include "svn_private_config.h"
> > +
> > /* The cache object. */
> > struct svn_cache_t {
> > /* Maps from a key (of size CACHE->KLEN) to a struct cache_entry. */
> > @@ -55,6 +59,13 @@ struct svn_cache_t {
> > * structs, as well as the dup'd values and hash keys.
> > */
> > apr_pool_t *cache_pool;
> > +
> > +#if APR_HAS_THREADS
> > + /* A lock for intra-process synchronization to the cache, or NULL if
> > + * the cache's creator doesn't feel the cache needs to be
> > + * thread-safe. */
> > + apr_thread_mutex_t *mutex;
> > +#endif
> > };
> >
> > /* A cache page; all items on the page are allocated from the same
> > @@ -115,7 +126,17 @@ svn_cache_create(svn_cache_t **cache_p,
> > /* The sentinel doesn't need a pool. (We're happy to crash if we
> > * accidentally try to treat it like a real page.) */
> >
> > - /* ### TODO: mutex */
> > +#if APR_HAS_THREADS
> > + if (thread_safe)
> > + {
> > + apr_status_t status = apr_thread_mutex_create(&(cache->mutex),
> > + APR_THREAD_MUTEX_DEFAULT,
> > + pool);
> > + if (status)
> > + return svn_error_wrap_apr(status,
> > + _("Can't create cache mutex"));
> > + }
> > +#endif
> >
> > cache->cache_pool = pool;
> >
> > @@ -188,6 +209,41 @@ duplicate_key(svn_cache_t *cache,
> > return apr_pmemdup(pool, key, cache->klen);
> > }
> >
> > +/* If applicable, locks CACHE's mutex. */
> > +static svn_error_t *
> > +lock_cache(svn_cache_t *cache)
> > +{
> > +#if APR_HAS_THREADS
> > + apr_status_t status;
> > + if (! cache->mutex)
> > + return SVN_NO_ERROR;
> > +
> > + status = apr_thread_mutex_lock(cache->mutex);
> > + if (status)
> > + return svn_error_wrap_apr(status, _("Can't lock cache mutex"));
> > +#endif
> > +
> > + return SVN_NO_ERROR;
> > +}
> > +
> > +/* If applicable, unlocks CACHE's mutex, then returns ERR. */
> > +static svn_error_t *
> > +unlock_cache(svn_cache_t *cache,
> > + svn_error_t *err)
> > +{
> > +#if APR_HAS_THREADS
> > + apr_status_t status;
> > + if (! cache->mutex)
> > + return err;
> > +
> > + status = apr_thread_mutex_unlock(cache->mutex);
> > + if (status && !err)
> > + return svn_error_wrap_apr(status, _("Can't unlock cache mutex"));
> > +#endif
> > +
> > + return err;
> > +}
> > +
> > svn_error_t *
> > svn_cache_get(void **value_p,
> > svn_boolean_t *found,
> > @@ -195,25 +251,26 @@ svn_cache_get(void **value_p,
> > const void *key,
> > apr_pool_t *pool)
> > {
> > - /* ### TODO: mutex */
> > -
> > - void *entry_void = apr_hash_get(cache->hash, key, cache->klen);
> > + void *entry_void;
> > struct cache_entry *entry;
> > + svn_error_t *err;
> > +
> > + SVN_ERR(lock_cache(cache));
> >
> > + entry_void = apr_hash_get(cache->hash, key, cache->klen);
> > if (! entry_void)
> > {
> > *found = FALSE;
> > - return SVN_NO_ERROR;
> > + return unlock_cache(cache, SVN_NO_ERROR);
> > }
> >
> > entry = entry_void;
> >
> > move_page_to_front(cache, entry->page);
> >
> > - SVN_ERR(duplicate_value(value_p, cache, entry->value, pool));
> > *found = TRUE;
> > -
> > - return SVN_NO_ERROR;
> > + err = duplicate_value(value_p, cache, entry->value, pool);
> > + return unlock_cache(cache, err);
> > }
> >
> > /* Removes PAGE from the LRU list, removes all of its entries from
> > @@ -252,7 +309,12 @@ svn_cache_set(svn_cache_t *cache,
> > void *value,
> > apr_pool_t *pool)
> > {
> > - void *existing_entry = apr_hash_get(cache->hash, key, cache->klen);
> > + void *existing_entry;
> > + svn_error_t *err = SVN_NO_ERROR;
> > +
> > + SVN_ERR(lock_cache(cache));
> > +
> > + existing_entry = apr_hash_get(cache->hash, key, cache->klen);
> >
> > /* Is it already here, but we can do the one-item-per-page
> > * optimization? */
> > @@ -281,8 +343,8 @@ svn_cache_set(svn_cache_t *cache,
> > struct cache_page *page = entry->page;
> >
> > move_page_to_front(cache, page);
> > - SVN_ERR(duplicate_value(&(entry->value), cache, value, page->page_pool));
> > - return SVN_NO_ERROR;
> > + err = duplicate_value(&(entry->value), cache, value, page->page_pool);
> > + goto cleanup;
> > }
> >
> > /* Do we not have a partial page to put it on, but we are allowed to
> > @@ -318,8 +380,10 @@ svn_cache_set(svn_cache_t *cache,
> >
> > /* Copy the key and value into the page's pool. */
> > new_entry->key = duplicate_key(cache, key, page->page_pool);
> > - SVN_ERR(duplicate_value(&(new_entry->value), cache, value,
> > - page->page_pool));
> > + err = duplicate_value(&(new_entry->value), cache, value,
> > + page->page_pool);
> > + if (err)
> > + goto cleanup;
> >
> > /* Add the entry to the page's list. */
> > new_entry->page = page;
> > @@ -341,6 +405,6 @@ svn_cache_set(svn_cache_t *cache,
> > }
> > }
> >
> > - /* ### TODO: mutex */
> > - return SVN_NO_ERROR;
> > + cleanup:
> > + return unlock_cache(cache, err);
> > }
>
--
David Glasser | glasser@davidglasser.net | http://www.davidglasser.net/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org