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

Re: Subversion BDB doesn't work with Apache 2.4 event MPM

From: Philip Martin <philip.martin_at_wandisco.com>
Date: Wed, 04 Apr 2012 23:44:58 +0100

Philip Martin <philip.martin_at_wandisco.com> writes:

> I think there is a refcount/locking bug in svn_fs_bdb__close. This code
>
> - if (0 == --bdb_baton->error_info->refcount && bdb->pool)
> - {
> - svn_error_clear(bdb_baton->error_info->pending_errors);
> -#if APR_HAS_THREADS
> - free(bdb_baton->error_info);
> - apr_threadkey_private_set(NULL, bdb->error_info);
> -#endif
> - }
>
> should be inside svn_fs_bdb__close_internal protected by the
> bdb_cache_lock otherwise the error_info refcount can change while
> another thread is inside svn_fs_bdb__open_internal and holding the lock.

That's not correct--the error data is per-thread and so there should be
no race. Except ...

> However moving the code from __close to __close_internal so it is inside
> the lock doesn't stop the tests failing so there must be a second bug
> somewhere.

... I think I may have identified the problem. The patch below checks
that the error struct is allocated and released by the same thread.
With the worker MPM the assertion always passes but with the event MPM I
get assertion failures. I don't fully understand the effect this would
have but I suspect it explains the problem.

Index: subversion/libsvn_fs_base/bdb/env.c
===================================================================
--- subversion/libsvn_fs_base/bdb/env.c (revision 1309593)
+++ subversion/libsvn_fs_base/bdb/env.c (working copy)
@@ -186,6 +186,7 @@ get_error_info(const bdb_env_t *bdb)
   if (!priv)
     {
       priv = calloc(1, sizeof(bdb_error_info_t));
+ ((struct bdb_error_info_t *)priv)->id = pthread_self();
       apr_threadkey_private_set(priv, bdb->error_info);
     }
   return priv;
@@ -524,6 +525,7 @@ svn_fs_bdb__close(bdb_env_baton_t *bdb_baton)
 
   SVN_ERR_ASSERT(bdb_baton->env == bdb_baton->bdb->env);
   SVN_ERR_ASSERT(bdb_baton->error_info->refcount > 0);
+ SVN_ERR_ASSERT(pthread_equal(bdb_baton->error_info->id, pthread_self()));
 
   /* Neutralize bdb_baton's pool cleanup to prevent double-close. See
      cleanup_env_baton(). */
Index: subversion/libsvn_fs_base/bdb/env.h
===================================================================
--- subversion/libsvn_fs_base/bdb/env.h (revision 1309593)
+++ subversion/libsvn_fs_base/bdb/env.h (working copy)
@@ -71,6 +71,8 @@ typedef struct bdb_error_info_t
      instances that refer to this object. */
   unsigned refcount;
 
+ pthread_t id;
+
 } bdb_error_info_t;
 

-- 
uberSVN: Apache Subversion Made Easy
http://www.uberSVN.com
Received on 2012-04-05 00:45:37 CEST

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