Mailing-List: contact dev-help@subversion.apache.org; run by ezmlm
Precedence: bulk
Received-SPF: pass (athena.apache.org: domain of evgeny.kotkov@visualsvn.com
 designates 209.85.217.182 as permitted sender)
MIME-Version: 1.0
In-Reply-To: <20150211174807.615BDAC006C@hades.apache.org>
References: <20150211174807.615BDAC006C@hades.apache.org>
From: Evgeny Kotkov <evgeny.kotkov@visualsvn.com>
Date: Thu, 12 Feb 2015 15:26:57 +0300
Message-ID: 
 <CAP_GPNjF=0BA8jxK8yNuUGXtkW8aRtLZ_Y9Fek=WaMudr=CEcA@mail.gmail.com>
Subject: Re: svn commit: r1659013 -
 /subversion/trunk/subversion/libsvn_subr/dso.c
To: philip@apache.org
Cc: Subversion Development <dev@subversion.apache.org>
Content-Type: multipart/mixed; boundary=089e0158b6f8b1132c050ee33d97

--089e0158b6f8b1132c050ee33d97
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable

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, t=
his
change avoids segfaults with --enable-runtime-module-search, but there is m=
ore
to it.  Documentation for svn_dso_initialize2() states the following (by th=
e
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 =E2=80=94 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

--089e0158b6f8b1132c050ee33d97
Content-Type: text/plain; charset=US-ASCII;
	name="svn-tests-initialize-everything-v1.patch.txt"
Content-Disposition: attachment;
	filename="svn-tests-initialize-everything-v1.patch.txt"
Content-Transfer-Encoding: base64
X-Attachment-Id: f_i624ehh10

SW5kZXg6IHN1YnZlcnNpb24vdGVzdHMvc3ZuX3Rlc3RfbWFpbi5jDQo9PT09PT09PT09PT09PT09
PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09DQotLS0g
c3VidmVyc2lvbi90ZXN0cy9zdm5fdGVzdF9tYWluLmMJKHJldmlzaW9uIDE2NTkyMTcpDQorKysg
c3VidmVyc2lvbi90ZXN0cy9zdm5fdGVzdF9tYWluLmMJKHdvcmtpbmcgY29weSkNCkBAIC00Niw2
ICs0Niw3IEBADQogI2luY2x1ZGUgInN2bl9jdHlwZS5oIgogI2luY2x1ZGUgInN2bl91dGYuaCIK
ICNpbmNsdWRlICJzdm5fdmVyc2lvbi5oIgorI2luY2x1ZGUgInN2bl9mcy5oIgogCiAjaW5jbHVk
ZSAicHJpdmF0ZS9zdm5fY21kbGluZV9wcml2YXRlLmgiCiAjaW5jbHVkZSAicHJpdmF0ZS9zdm5f
YXRvbWljLmgiCkBAIC03NjIsMTIgKzc2Myw4IEBAIHN2bl90ZXN0X21haW4oaW50IGFyZ2MsIGNv
bnN0IGNoYXIgKmFyZ3ZbXSwgaW50IG1hDQogCiAgIG9wdHMuZnNfdHlwZSA9IERFRkFVTFRfRlNf
VFlQRTsKIAotICAvKiBJbml0aWFsaXplIEFQUiAoQXBhY2hlIHBvb2xzKSAqLwotICBpZiAoYXBy
X2luaXRpYWxpemUoKSAhPSBBUFJfU1VDQ0VTUykKLSAgICB7Ci0gICAgICBwcmludGYoImFwcl9p
bml0aWFsaXplKCkgZmFpbGVkLlxuIik7Ci0gICAgICBleGl0KDEpOwotICAgIH0KKyAgaWYgKHN2
bl9jbWRsaW5lX2luaXQoInN2bl90ZXN0cyIsIHN0ZGVycikgIT0gRVhJVF9TVUNDRVNTKQorICAg
IGV4aXQoMSk7CiAKICAgLyogc2V0IHVwIHRoZSBnbG9iYWwgcG9vbC4gIFVzZSBhIHNlcGFyYXRl
IGFsbG9jYXRvciB0byBsaW1pdCBtZW1vcnkKICAgICogdXNhZ2UgYnV0IG1ha2UgaXQgdGhyZWFk
LXNhZmUgdG8gYWxsb3cgZm9yIG11bHRpLXRocmVhZGVkIHRlc3RzLgpAQCAtNzg0LDYgKzc4MSwx
MyBAQCBzdm5fdGVzdF9tYWluKGludCBhcmdjLCBjb25zdCBjaGFyICphcmd2W10sIGludCBtYQ0K
ICAgdGVzdF9hcmdjID0gYXJnYzsKICAgdGVzdF9hcmd2ID0gYXJndjsKIAorICBlcnIgPSBzdm5f
ZnNfaW5pdGlhbGl6ZShwb29sKTsKKyAgaWYgKGVycikKKyAgICB7CisgICAgICBzdm5faGFuZGxl
X2Vycm9yMihlcnIsIHN0ZGVyciwgVFJVRSwgInN2bl90ZXN0czogIik7CisgICAgICBzdm5fZXJy
b3JfY2xlYXIoZXJyKTsKKyAgICB9CisKICAgZXJyID0gaW5pdF90ZXN0X2RhdGEoYXJndlswXSwg
cG9vbCk7CiAgIGlmIChlcnIpCiAgICAgewo=
--089e0158b6f8b1132c050ee33d97--