Philip Martin <[email protected]> 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?
Regards,
Evgeny Kotkov
Index: subversion/tests/svn_test_main.c
===================================================================
--- subversion/tests/svn_test_main.c (revision 1659217)
+++ subversion/tests/svn_test_main.c (working copy)
@@ -46,6 +46,7 @@
#include "svn_ctype.h"
#include "svn_utf.h"
#include "svn_version.h"
+#include "svn_fs.h"
#include "private/svn_cmdline_private.h"
#include "private/svn_atomic.h"
@@ -762,12 +763,8 @@ svn_test_main(int argc, const char *argv[], int ma
opts.fs_type = DEFAULT_FS_TYPE;
- /* Initialize APR (Apache pools) */
- if (apr_initialize() != APR_SUCCESS)
- {
- printf("apr_initialize() failed.\n");
- exit(1);
- }
+ if (svn_cmdline_init("svn_tests", stderr) != EXIT_SUCCESS)
+ exit(1);
/* set up the global pool. Use a separate allocator to limit memory
* usage but make it thread-safe to allow for multi-threaded tests.
@@ -784,6 +781,13 @@ svn_test_main(int argc, const char *argv[], int ma
test_argc = argc;
test_argv = argv;
+ err = svn_fs_initialize(pool);
+ if (err)
+ {
+ svn_handle_error2(err, stderr, TRUE, "svn_tests: ");
+ svn_error_clear(err);
+ }
+
err = init_test_data(argv[0], pool);
if (err)
{
