On Wed, Jan 11, 2017 at 6:17 PM, Jim Jagielski <[email protected]> wrote: > >> On Jan 11, 2017, at 12:12 PM, Joe Orton <[email protected]> wrote: >> >>> The only reason why I can see why the orig idea to use s->process->pool >>> was to make watchdog run independent of any restarts of httpd >>> itself... that is, a truly independent watchdog. But that would >>> imply that you can't reconfig watchdog on restarts which >>> doesn't seem quite right...
Some (third party) modules may depend on this, no? >> >> Since the callbacks registered with the watchdog come from modules, and >> modules have lifetime of pconf, it seems right to me to use pconf. (I >> would guess that without this fix, if at restart you unloaded a module >> which had registered a watchdog, it would segfault the server?) Modules could "mimic" mod_watchdog behaviour (by using process->pool and recover their context at restart)... But I don't know any such module, just noting about the behaviour change if we use pconf here. >> >> At restart time when we re-enter the main() loop and clear pconf, >> that'll run the pre_cleanup wd_worker_cleanup() for each registered >> worker. That invokes apr_thread_join() which waits for the worker >> thread to terminate. I guess that is sufficient because each thread >> will then query the MPM state, see "AP_MPMQ_STOPPING", and terminate. >> >> Not very confident in that though. Unfortunately wd_worker_cleanup() is registered on process->pool too, the one passed to wd_startup(). > > Doing some more testing, I can't see offhand any "reasonable" > way to use s->process->pool as the parent pool without a > lot of nasty, ugly hacks that ignore any but the 2nd (re)start, > etc... which would cause us to leak memory in any case. Agreed, mod_watchdog leaks. > > So I think the fix is *the* fix. > > Will propose for backport in a bit. I hope it won't break anyone. Could we at least s/pproc/pconf/ in post_config along with this commit?
