On 04.03.2015 14:31, Ivan Zhakov wrote:
> On 4 March 2015 at 16:07, Branko Čibej <brane_at_wandisco.com> wrote:
>> On 04.03.2015 13:50, Ivan Zhakov wrote:
>>> On 4 March 2015 at 11:27, Branko Čibej <brane_at_wandisco.com> wrote:
>>>> There was some discussion on #svn-dev recently about making the error
>>>> location tracing (see libsvn_subr/error.c and svn_error__locate)
>>>> thread-safe, by using platform- and compiler-specific flags for making
>>>> the location variables thread-safe.
>>>>
>>>> It turns out that, since we now require APR 1.3+ which provides
>>>> unmanaged pools, we can now safely do this by using APR's threadkey API.
>>>> See:
>>>>
>>>> https://paste.apache.org/fG82
>>>>
>>>> With this patch, all tests pass for on my Mac in parallel mode and the
>>>> error stacks look sane. But, before committing the change, I'd like
>>>> someone else to review the patch because it's just a wee bit tricky in
>>>> places what with all the #ifdefs.
>>>>
>>> I was thinking about similar change, but as far I remember it's not
>>> safe to use svn_atomic__init_once() in error.c, because
>>> svn_atomic__init_once() uses svn_error_*() API. This may lead circular
>>> initialization of svn_error_*() tracing infrastructure.
>> Nope. svn_atomic__init_once only begins using the svn_error_* functions
>> after the init function has returned, at which point the tracing logic
>> is initialized. Furthermore, I intentionally implemented this particular
>> init function so that it never returns an error.
>>
>> Although, looking at the implementation of svn_atomic__init_once, we
>> really shouldn't be optimizing the error case with all the #if
>> APR_HAS_THREADS blocks.
>>
> It may be doesn't cause problems now, but svn_atomic__init_once()
> implementation may change in future. So I really prefer to
> re-implement svn_atomic__init_once() in error.c. It should not be big
> problem IMO, but could avoid some weird deadlocks in future.
I don't believe it's worth the trouble, and even less worth the code
duplication. Consider the following:
* If svn_atomic__init_once does anything but spin in other threads
while the init function is running, it's broken.
* Before the init function is called, and also after it completes, the
error location bits are guaranteed to be in a valid state.
So the only way to break anything would be if svn_atomic__init_once is
broken, either due to doing things it shouldn't or allowing some other
thread to pass through while the init-func is running.
It's a definite waste of effort to duplicate code and maintain it
forever on the off chance that someone is going to "optimize"
svn_atomic__init_once so that doesn't follow its declared contract. Lots
of other things will break if that happens.
-- Brane
Received on 2015-03-04 14:42:16 CET