On 11 February 2015 at 20:48, <philip_at_apache.org> wrote:
> Author: philip
> Date: Wed Feb 11 17:48:06 2015
> New Revision: 1659013
>
> URL: http://svn.apache.org/r1659013
> Log:
> Wrap svn_dso_initialize2 call with svn_atomic__init_once, this
> fixes a crash in the DSO hash code when running the C tests in
> parallel.
>
> * subversion/libsvn_subr/dso.c
> (atomic_init_func): New.
> (svn_dso_load): Use svn_atomic__init_once.
>
> Modified:
> subversion/trunk/subversion/libsvn_subr/dso.c
>
> Modified: subversion/trunk/subversion/libsvn_subr/dso.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_subr/dso.c?rev=1659013&r1=1659012&r2=1659013&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_subr/dso.c (original)
> +++ subversion/trunk/subversion/libsvn_subr/dso.c Wed Feb 11 17:48:06 2015
> @@ -28,6 +28,7 @@
> #include "svn_private_config.h"
>
> #include "private/svn_mutex.h"
> +#include "private/svn_atomic.h"
>
> /* A mutex to protect our global pool and cache. */
> static svn_mutex__t *dso_mutex = NULL;
> @@ -41,6 +42,8 @@ static apr_hash_t *dso_cache;
> /* Just an arbitrary location in memory... */
> static int not_there_sentinel;
>
> +static volatile svn_atomic_t atomic_init_status = 0;
> +
> /* A specific value we store in the dso_cache to indicate that the
> library wasn't found. This keeps us from allocating extra memory
> from dso_pool when trying to find libraries we already know aren't
> @@ -61,6 +64,14 @@ svn_dso_initialize2(void)
> return SVN_NO_ERROR;
> }
>
> +static svn_error_t *
> +atomic_init_func(void *baton,
> + apr_pool_t *pool)
> +{
> + SVN_ERR(svn_dso_initialize2());
> + return SVN_NO_ERROR;
> +}
> +
> #if APR_HAS_DSO
> static svn_error_t *
> svn_dso_load_internal(apr_dso_handle_t **dso, const char *fname)
> @@ -107,8 +118,8 @@ svn_dso_load_internal(apr_dso_handle_t *
> svn_error_t *
> svn_dso_load(apr_dso_handle_t **dso, const char *fname)
> {
> - if (! dso_pool)
> - SVN_ERR(svn_dso_initialize2());
> + SVN_ERR(svn_atomic__init_once(&atomic_init_status, atomic_init_func,
> + NULL, NULL));
>
Philip,
The fix itself makes sense, but I agree with Evgeny that long-term fix
should be add call to svn_dso_initialize2() in the test suite like we
do in all application and mod_dav_svn.
Also current fix doesn't protect from svn_dso_initialize2() executing
concurrently if one thread call svn_dso_initialize2() directly, while
other thread will call it indirectly through svn_dso_load().
Why you didn't make svn_atomic__init_once() internal implementation
details of svn_dso_initialize2()?
--
Ivan Zhakov
Received on 2015-02-12 17:02:00 CET