On 12 February 2015 at 15:42, Branko Čibej <br...@wandisco.com> wrote: > On 12.02.2015 13:26, Evgeny Kotkov wrote: >> Philip Martin <phi...@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
