Re: svn commit: r1778319 - /httpd/httpd/trunk/modules/core/mod_watchdog.c
> On Jan 11, 2017, at 12:28 PM, Yann Ylavic wrote: > > On Wed, Jan 11, 2017 at 6:17 PM, Jim Jagielski wrote: >> >>> On Jan 11, 2017, at 12:12 PM, Joe Orton 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. > I don't think the intent was such... looking at everything else, the design of watchdog is consistent with a typical pconf/clean on restarts and the initial use of s->process->pool was a bug.
Re: svn commit: r1778319 - /httpd/httpd/trunk/modules/core/mod_watchdog.c
On Wed, Jan 11, 2017 at 6:17 PM, Jim Jagielski wrote: > >> On Jan 11, 2017, at 12:12 PM, Joe Orton 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?
Re: svn commit: r1778319 - /httpd/httpd/trunk/modules/core/mod_watchdog.c
> On Jan 11, 2017, at 12:12 PM, Joe Orton wrote: > > On Wed, Jan 11, 2017 at 11:08:29AM -0500, Jim Jagielski wrote: >> This is to address the following bug: >> >> https://bugzilla.redhat.com/show_bug.cgi?id=1410883 > > Thanks a lot Jim! > >> 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... > > 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?) > > 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. > 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. So I think the fix is *the* fix. Will propose for backport in a bit.
Re: svn commit: r1778319 - /httpd/httpd/trunk/modules/core/mod_watchdog.c
On Wed, Jan 11, 2017 at 11:08:29AM -0500, Jim Jagielski wrote: > This is to address the following bug: > > https://bugzilla.redhat.com/show_bug.cgi?id=1410883 Thanks a lot Jim! > 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... 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?) 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.
Re: svn commit: r1778319 - /httpd/httpd/trunk/modules/core/mod_watchdog.c
This is to address the following bug: https://bugzilla.redhat.com/show_bug.cgi?id=1410883 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... thoughts? > On Jan 11, 2017, at 11:00 AM, [email protected] wrote: > > Author: jim > Date: Wed Jan 11 16:00:37 2017 > New Revision: 1778319 > > URL: http://svn.apache.org/viewvc?rev=1778319&view=rev > Log: > Use pconf as parent pool so mutexes get cleaned on restarts/reloads > > Modified: >httpd/httpd/trunk/modules/core/mod_watchdog.c > > Modified: httpd/httpd/trunk/modules/core/mod_watchdog.c > URL: > http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/core/mod_watchdog.c?rev=1778319&r1=1778318&r2=1778319&view=diff > == > --- httpd/httpd/trunk/modules/core/mod_watchdog.c (original) > +++ httpd/httpd/trunk/modules/core/mod_watchdog.c Wed Jan 11 16:00:37 2017 > @@ -436,7 +436,7 @@ static int wd_post_config_hook(apr_pool_ > { > apr_status_t rv; > const char *pk = "watchdog_init_module_tag"; > -apr_pool_t *pproc = s->process->pool; > +apr_pool_t *pproc = pconf; > const apr_array_header_t *wl; > > if (ap_state_query(AP_SQ_MAIN_STATE) == AP_SQ_MS_CREATE_PRE_CONFIG) > >
