On Thu, Jul 26, 2018 at 3:35 PM, Branko Čibej <br...@apache.org> wrote:
> On 25.07.2018 17:46, William A Rowe Jr wrote:
>> Giving this problem some thought...
>>
>> If an aprfoo-1.so provider is linked to the libfoo.so library, and we
>> track the init state and perform it only once, it seems that apr
>> should never try to de-init or unload the .so provider for the
>> lifespan of the process.
>>
>> This should help us avoid some redundant initialization. While we
>> could free resources atexit()
>
> Shared libraries (remember, APR is usually a shared lib too) should
> avoid using the atexit() mechanism. It's a good way to shoot yourself in
> the foot; on Unix, you *may*, depending on the platform, have the
> atexit() handlers run in a predictable order. But on Windows, each DLL
> has its own atexit() queue and all predictability goes out the window.
+1 on this, plus we can't have an atexit handler/code in the DSO
itself (can't be called after the DSO is unloaded).
There are few garantees with atexit, and things like 'attribute
((destructor))' are not portable.
But unloading is not the issue IMO, more below.
>
>> , any spin of trying to repeatedly unload and reload any .so provider
>> seems to be inherently messy and unstable. We see this issue with
>> mod_ssl, etc, where the statics may be freed and reinitialized, and
>> causes the resource leakage mentioned by Yann.
>
> Yes, unloading a DSO that wasn't designed for it is always messy, and
> probably always wrong, too.
I think we should separate the (un)loading of the DSO and the (de)init
of the library in this discussion.
Loading the DSO usually does not init the lib (e.g. call
OPENSSL_init), and unloading does not deinit either (there may be
mechanisms in the lib that do the init on the first call any/some
function, and deinit with atexit-alike, but that's the lib's mistake,
IMHO).
Loading clears/zeros the statics though (when ++refcount == 1), which
is the only way to unload => reload/reinit with some libs.
If deiniting the lib isn't useful, there is indeed few interest in
unloading it. But otherwise we should do both.
I guess this is the user's decision, not APR's. Regarding openssl for
instance, I'd be more confortable if the lib were deinited when I ask
it to be, because there may be things in memory that I would not want
to leak (e.g. the RAND state, which if it ended in a core file -- say
this happens when the program stops -- could possibly allow one to
guess random bytes generated so far. I think openssl's RAND state is
cleared on deinit only, so I want to deinit at some point).
So what I'm concerned about is letting the user choose whether or not
APR should init/deinit a lib it uses; either "libfoo.so" if it's
linked, or "aprfoo-1.so" if it's loaded dynamically.
I'll describe two cases I can think of (there may be others, it's
maybe also a bit too httpd centric, but possibly the others cases if
any fall through these ones):
1/ the main program is using "libfoo.so" for itself, and it (or some
of its DSOs) also uses/loads "aprfoo-1.so" (via APR interface);
2/ the main program is not using "libfoo.so", but some of its DSOs use
"aprfoo-1.so" independendly (the DSOs usually know nothing of each
other).
In case 1/, "aprfoo-1.so" should not init nor deinit, it's the main
program's business.
In case 2/, each DSO's "aprfoo-1.so" should/will (try/ask to) init and
deinit the lib.
In both cases the main program and DSOs want to cooperate through APR,
this is what we are working on/for ;)
I think we can address this with the right interface(s), and pools still.
APR has already the code to load/unload "aprfoo-1.so", and it does it
automatically with the global pool's lifetime (almost).
APR also has the code to init/deinit "libfoo.so" for its internal use,
and it does it with the same global pool/lifetime.
My opinion is that we should provide both interfaces to the user, each
using the given pool for its own lifetime:
- apr_lib_load("foo", ..., load_pool) which loads (and registers unload) once,
- apr_lib_init("foo", ..., init_pool) which inits (and registers deinit) once.
The load/init pools are not necessarily different, user's choice, and
the "once" would take ancestors into account.
Also, apr_lib_load("foo") would call apr_lib_init("foo") like we do
currently, with an opt-out option.
(Note that for e.g. APR crypto, apr_lib_load is actually
apr_crypto_get_driver(), and apr_lib_init() is the disputed/vetoed
apr_crypto_lib_init(), I'm just using consistent/generic/short names
here)
Now I think the user can adapt to use cases 1/ or 2/. Yes (s)he can do
wrong things with inconsistent load/init pools, but it's no different
than other APR interfaces (and there probably can be relevant
pool_is_ancestor checks to avoid this).
Does it sound wrong or miss uses cases (notably in subversion)?
Back to 1/ and 2/ (for now..).
Case 1:
The main program links/loads "libfoo.so" by design (for it's own use)
and thus can call apr_lib_init("foo", global_pool) for that.
It or its DSOs also use "aprfoo-1.so" so they call apr_lib_load("foo",
global_or_sub-maybe-cleared-on-reload_pool) for that. This will load
"libfoo.so" (actually ++refcount only), but not init/deinit spuriously
since the global_pool tracks one init already.
So we are fine, and the main program is now also released from
initializing libfoo, with foo*() functions, by itself. For e.g. for
openssl it's nice to have this across versions, and APR has to have it
anyway.
Case 2:
The main program does nothing w.r.t. libfoo.
Its DSOs somehow call apr_lib_load("foo",
all_the_same_sub-maybe-cleared-on-reload_pool), which inits once (on
the first call), and deinits once (just before "aprfoo-1.so" is
finally unloaded), thanks to the implicit apr_lib_init("foo").
We are fine still I think.
In both cases the pools do their job, and the user controls the lifetime.
>>
>> Once loaded and initialized, we recieve no benefit from repeatedly
>> altering the processes' library mmaps, and doing so after we spin up
>> contending threads is an even worse scenario. Leaving them loaded
>> should not have an adverse effect, and if it becomes swapped out due
>> to lack of use during the lifespan of the process, should not be
>> particularly harmful.
Well, I don't really mind about memory usage for not-unloaded DSOs,
but more about not-deinited.
And deinit should be followed by unload, otherwise for libs with
statics (like openssl) there is no possible reinit (while
reload/reinit works).
>>
>> While httpd modules are often unloaded and reloaded, the underlying
>> libfoo.so is still stuck in process to libapr-1.so, so those should
>> remain stable.
Speaking of httpd specifically, I see no issue with the interfaces I
proposed above.
It can even work for modules linked statically since in this case the
module can apr_lib_init() on the global pool (i.e. once for the
lifetime of httpd).
>>
>> Does this make sense?
>
> Yes, however: apr_dso_load() takes a pool as context, and when the pool
> is destroyed, all the related DSO modules are unloaded, whether you want
> them to be or not. Even using a global pool for apr_dso_load() does not
> help because you still will have no control of the lifetime of other
> pools or code that may still be live and depend on the DSO module when
> it's unloaded at global pool cleanup time.
For me the unconditional APR global pool is an issue per se.
I don't know enough about subversion to give a relevant opinion, but
supposedly it would be much easier for subversion to control when
"aprfoo-1.so" loads/unloads and/or inits/deinits, according to
subversion's workflow.
>
> We've had constant and painful problems with this in Subversion, having
> to track down random crashes at process exit. We've implemented really
> horrible workarounds, and I'm still not sure we've covered every edge case.
Supposedly still, this may be because of a DSO cleanup being called
after the DSO is unloaded (i.e. cleanup code already unmapped).
I'd blame the global pool for this, for not being the one you'd want
to destroy/cleanup at the right time.
Regards,
Yann.
