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.