apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Ken Been <kbb...@gmail.com>
Subject Re: thread safety of apr_initialize
Date Sun, 22 Sep 2013 15:30:10 GMT
Thanks for the quick response.

I would like to avoid requiring a call to a single-threaded global init
function, since it's a viral requirement that would apply to any libraries
that use mine, and any libraries that use those, etc.  If an old library
wanted to start using mine then it would likely have to make an
incompatible change to its API.  In fact, the only reason I started
considering serf was that I wanted to avoid this requirement in curl.

I'm not quite convinced that making the entire apr_initialize a critical
section wouldn't solve the problem.  Can you expand on that a little?  What
could go wrong?  I browsed the source code a bit (the unix part), and I saw
mostly memory allocation and assignment.  None of that should cause a
problem.  (I didn't, for example, see calls to external libraries, which is
curl's problem.)

Ken

On Sat, Sep 21, 2013 at 11:59 PM, Branko ─îibej <brane@apache.org> wrote:

> On 22.09.2013 04:06, Ken Been wrote:
> > I am considering using a library (serf) built on APR in my own
> > library.  Since I'm writing a library, I can make no assumptions about
> > what the calling program does - it might have multiple threads started
> > before it calls my library, and some of those threads might be using APR.
> >
> > My library can call apr_initialize, but I can't guarantee that it
> > won't conflict with another call to apr_initialize in some other
> > thread that I don't control.  Since apr_initialize doesn't seem to be
> > thread safe, and I don't want to put (what I consider to be) onerous
> > requirements on the users of my library, this effectively makes APR
> > unusable for me.
>
> If the application that uses your library also uses APR directly, it's
> already responsible for initializing it in a single-threaded context. If
> it doesn't, it's reasonable to require the app to call your library's
> init function, which you can easily protect with a spinlock (that you
> can implement using APR's atomic functions).
>
> > So first, is my analysis correct?
>
> Correct as far as it goes, but incomplete.
>
> >   And second, if it is, is there some good reason that apr_initialize
> > has not been made thread safe?  At first glance I think simply making
> > "initialized++" an atomic operation should do it,
>
> No, that wouldn't be enough. Just making the counter increment in
> apr_initialize (and the decrement in apr_terminate) atomic will not
> prevent race conditions. If two threads call apr_initialize
> simultaneously, one of them could return with the assumption that APR
> was initialized, while the second is still running the initialization
> code. Hence the requirement that apr_initialize must be called in a
> single-threaded context.
>
> That said, it would still make sense to make the counter itself atomic,
> if only to avoid unexpected results on architectures that do not
> guarantee atomic reads and writes to word-sized memory locations.
>
> > or if not then making the whole function body a critical section.  I'd
> > be happy to work on a patch, but the fact that this hasn't been done
> > makes me suspect there must be a good reason for that.
>
> APR doesn't make assumptions about how an application uses it. So it's
> up to the caller to make sure the initialization happens in a
> single-threaded context.
>
> That said ... the single-threaded initialization requirement isn't as
> onerous as all that; it only requires the application to add two lines
> of code in its main function (ignoring error handling):
>
>     apr_initialize();
>     atexit(apr_terminate());
>
> Any non-trivial application will already have a ton of single-threaded
> initialization code anyway, so the above will hardly make any difference.
>
> If you don't want to expose the dependency on APR from your library, you
> can create your own init function that the application would call
> instead, and do the error handling and atexit(...) call from that.
>
> -- Brane
>
>

Mime
View raw message