httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Yann Ylavic <ylavic....@gmail.com>
Subject Re: svn commit: r1778319 - /httpd/httpd/trunk/modules/core/mod_watchdog.c
Date Wed, 11 Jan 2017 17:28:15 GMT
On Wed, Jan 11, 2017 at 6:17 PM, Jim Jagielski <jim@jagunet.com> wrote:
>
>> On Jan 11, 2017, at 12:12 PM, Joe Orton <jorton@redhat.com> 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?

Mime
View raw message