Mailing-List: contact dev-help@subversion.apache.org; run by ezmlm
Precedence: bulk
Received-SPF: pass (nike.apache.org: domain of brane@wandisco.com designates
 209.85.212.173 as permitted sender)
Message-ID: <54DC9FAC.6050104@wandisco.com>
Date: Thu, 12 Feb 2015 13:42:20 +0100
From: =?UTF-8?B?QnJhbmtvIMSMaWJlag==?= <brane@wandisco.com>
Organization: WANdisco
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10;
 rv:31.0) Gecko/20100101 Thunderbird/31.4.0
MIME-Version: 1.0
To: dev@subversion.apache.org
Subject: Re: svn commit: r1659013 -
 /subversion/trunk/subversion/libsvn_subr/dso.c
References: <20150211174807.615BDAC006C@hades.apache.org>
 <CAP_GPNjF=0BA8jxK8yNuUGXtkW8aRtLZ_Y9Fek=WaMudr=CEcA@mail.gmail.com>
In-Reply-To: 
 <CAP_GPNjF=0BA8jxK8yNuUGXtkW8aRtLZ_Y9Fek=WaMudr=CEcA@mail.gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 8bit

On 12.02.2015 13:26, Evgeny Kotkov wrote:
> Philip Martin <philip@apache.org> writes:
>
>> 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.
> I think we should also address this problem in our test suite.  As I see, this
> change avoids segfaults with --enable-runtime-module-search, but there is more
> to it.  Documentation for svn_dso_initialize2() states the following (by the
> way, the "will not be entirely thread safe" part is now outdated, right?):
>
>  * @note This should be called prior to the creation of any pool that
>  *       is passed to a function that comes from a DSO, otherwise you
>  *       risk having the DSO unloaded before all pool cleanup callbacks
>  *       that live in the DSO have been executed.  If it is not called
>  *       prior to @c svn_dso_load being used for the first time there
>  *       will be a best effort attempt made to initialize the subsystem,
>  *       but it will not be entirely thread safe and it risks running
>  *       into the previously mentioned problems with DSO unloading and
>  *       pool cleanup callbacks.
>
> I am not sure that taking some risks within the test suite is a good idea,
> and I think that we should call functions like svn_dso_initialize2() and
> svn_fs_initialize().  If we don't, everything might still work fine, but it
> also might not, and we could spend a lot of time debugging random failures
> if a test runs into one of these pitfalls happening due to the absense of
> explicit initialization.  We do call these initializers in mod_dav_svn, svn,
> svnserve — so why not also do the same in the test programs?
>
> I don't see a reason to use a custom way of bootstrapping things within
> svn_test_main(), as opposed to main() functions in svn, svnadmin and other
> command-line tools, and I attached a patch that brings this common approach
> to svn_test_main().  Quick inspection shows a couple of other programs that
> should probably do the same, e.g., atomic-ra-revprop-change.  However, I
> think this is less important, and that we could do it separately.
>
> What do you think?


We should make our implementation work without having to run the
initializer functions. This is a lot easier for API users. There's
almost nothing we can't do within an svn_atomic__init_once block. The
only problematic feature is creating pools in an unknown thread context,
because we won't know the pool destruction order. Even this problem has
been mostly solved in the BDB DB_REGISTER support code.

-- Brane