Re: Thread-safe error location in maintainer mode

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.

-- 
Ivan Zhakov