Mailing-List: contact dev-help@subversion.apache.org; run by ezmlm
Precedence: bulk
Received-SPF: pass (nike.apache.org: domain of bert@qqmail.nl designates
 209.85.212.179 as permitted sender)
From: "Bert Huijben" <bert@qqmail.nl>
To: "'Ivan Zhakov'" <ivan@visualsvn.com>,
	"'Subversion Development'" <dev@subversion.apache.org>
Cc: "'Philip Martin'" <philip.martin@wandisco.com>
References: <20150211174807.615BDAC006C@hades.apache.org>
 <CABw-3Yc2UB0aeT2Xt6Khtn+oOv2jR-U5C1-qd1SBNSPfs9vY7A@mail.gmail.com>
In-Reply-To: 
 <CABw-3Yc2UB0aeT2Xt6Khtn+oOv2jR-U5C1-qd1SBNSPfs9vY7A@mail.gmail.com>
Subject: RE: svn commit: r1659013 -
 /subversion/trunk/subversion/libsvn_subr/dso.c
Date: Thu, 12 Feb 2015 18:31:44 +0100
Message-ID: <023d01d046e9$c5f582b0$51e08810$@qqmail.nl>
MIME-Version: 1.0
Content-Type: text/plain;
	charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Thread-Index: AQLGaxgUKJyQGwWm9wTRu/jvgsbRtgIdxyacmvAO0RA=
Content-Language: nl


> -----Original Message-----
> From: Ivan Zhakov [mailto:ivan@visualsvn.com]
> Sent: donderdag 12 februari 2015 16:59
> To: Subversion Development
> Cc: Philip Martin
> Subject: Re: svn commit: r1659013 -
> /subversion/trunk/subversion/libsvn_subr/dso.c
>=20
> On 11 February 2015 at 20:48,  <philip@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?re
> v=3D1659013&r1=3D1659012&r2=3D1659013&view=3Ddiff
> >
> =
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> > --- 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 =3D 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 =3D 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,
>=20
> 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.

+1

Asked the same question yesterday (on irc).

> 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().
>=20
> Why you didn't make svn_atomic__init_once() internal implementation
> details of svn_dso_initialize2()?

+1

Good suggestions

	Bert