subversion-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Ivan Zhakov <i...@visualsvn.com>
Subject Re: svn commit: r1659013 - /subversion/trunk/subversion/libsvn_subr/dso.c
Date Thu, 12 Feb 2015 12:48:35 GMT
On 12 February 2015 at 15:42, Branko Čibej <brane@wandisco.com> wrote:
> 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.
>
We should, but sometimes we cannot. As far I understand documentation
for svn_dso_initialize2() is exactly that case:
[[[
 * @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.
 ]]]

Multi-threaded application that didn't call svn_dso_initialize2()
before creating any other pools will fail anyway under some
circumstances. The problem with cleanup handles, not with concurrent
initialization.

-- 
Ivan Zhakov

Mime
View raw message