You are viewing a plain text version of this content. The canonical link for it is here.
Posted to cvs@httpd.apache.org by ji...@apache.org on 2017/01/11 16:00:37 UTC
svn commit: r1778319 - /httpd/httpd/trunk/modules/core/mod_watchdog.c
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)
Re: svn commit: r1778319 -
/httpd/httpd/trunk/modules/core/mod_watchdog.c
Posted by Jim Jagielski <ji...@jaguNET.com>.
> On Jan 11, 2017, at 12:28 PM, Yann Ylavic <yl...@gmail.com> wrote:
>
> On Wed, Jan 11, 2017 at 6:17 PM, Jim Jagielski <ji...@jagunet.com> wrote:
>>
>>> On Jan 11, 2017, at 12:12 PM, Joe Orton <jo...@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.
>
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
Posted by Yann Ylavic <yl...@gmail.com>.
On Wed, Jan 11, 2017 at 6:17 PM, Jim Jagielski <ji...@jagunet.com> wrote:
>
>> On Jan 11, 2017, at 12:12 PM, Joe Orton <jo...@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?
Re: svn commit: r1778319 -
/httpd/httpd/trunk/modules/core/mod_watchdog.c
Posted by Jim Jagielski <ji...@jaguNET.com>.
> On Jan 11, 2017, at 12:12 PM, Joe Orton <jo...@redhat.com> 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
Posted by Joe Orton <jo...@redhat.com>.
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
Posted by Jim Jagielski <ji...@apache.org>.
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, jim@apache.org 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)
>
>
Re: svn commit: r1778319 -
/httpd/httpd/trunk/modules/core/mod_watchdog.c
Posted by Jim Jagielski <ji...@apache.org>.
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, jim@apache.org 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)
>
>