On 12.02.2015 16:58, Ivan Zhakov wrote: > On 11 February 2015 at 20:48, <phi...@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()?
Yup, Ivan's right; the atomic init has to be hidden away within svn_dso_initialize2's implementation, because this is a public function. As to the other point, unfortunately I don't see an alternative to adding explicit calls to dso-init just after apr_initialize() at this point. Lack of control over global pool destruction is a serious flaw in APR, but not one we can work around (in general) as long as we support APR-1.3+. -- Brane
