You are viewing a plain text version of this content. The canonical link for it is here.
Posted to cvs@httpd.apache.org by wr...@apache.org on 2012/04/10 06:05:23 UTC
svn commit: r1311569 - in /httpd/mod_fcgid/trunk: CHANGES-FCGID
docs/manual/mod/mod_fcgid.xml modules/fcgid/fcgid_conf.c
modules/fcgid/fcgid_conf.h modules/fcgid/fcgid_pm_win.c
modules/fcgid/fcgid_proc_win.c modules/fcgid/mod_fcgid.c
Author: wrowe
Date: Tue Apr 10 04:05:23 2012
New Revision: 1311569
URL: http://svn.apache.org/viewvc?rev=1311569&view=rev
Log:
Introduce FcgidWin32PreventOrphans directive on Windows to use OS
Job Control Objects to terminate all running fcgi's when the worker
process has been abruptly terminated.
[wrowe: I've changed the volume level on some error levels, moved
the rv into the error log schema, and moved all commentary over to
the actual conf assignment function. With the exception of style
cleanup, this remains almost entirely Thangaraj's creation. Also
have added simple docs.]
PR: 51078
Submitted by: Thangaraj AntonyCrouse <thangaraj gmail.com>
Modified:
httpd/mod_fcgid/trunk/CHANGES-FCGID
httpd/mod_fcgid/trunk/docs/manual/mod/mod_fcgid.xml
httpd/mod_fcgid/trunk/modules/fcgid/fcgid_conf.c
httpd/mod_fcgid/trunk/modules/fcgid/fcgid_conf.h
httpd/mod_fcgid/trunk/modules/fcgid/fcgid_pm_win.c
httpd/mod_fcgid/trunk/modules/fcgid/fcgid_proc_win.c
httpd/mod_fcgid/trunk/modules/fcgid/mod_fcgid.c
Modified: httpd/mod_fcgid/trunk/CHANGES-FCGID
URL: http://svn.apache.org/viewvc/httpd/mod_fcgid/trunk/CHANGES-FCGID?rev=1311569&r1=1311568&r2=1311569&view=diff
==============================================================================
--- httpd/mod_fcgid/trunk/CHANGES-FCGID [utf8] (original)
+++ httpd/mod_fcgid/trunk/CHANGES-FCGID [utf8] Tue Apr 10 04:05:23 2012
@@ -1,6 +1,11 @@
-*- coding: utf-8 -*-
Changes with mod_fcgid 2.3.7
+ *) Introduce FcgidWin32PreventOrphans directive on Windows to use OS
+ Job Control Objects to terminate all running fcgi's when the worker
+ process has been abruptly terminated. PR: 51078
+ [Thangaraj AntonyCrouse <thangaraj gmail.com>]
+
*) Periodically clean out the brigades which are pulling in the request
body for handoff to the fcgid child. PR: 51749
[Dominic Benson <dominic.benson thirdlight.com>]
Modified: httpd/mod_fcgid/trunk/docs/manual/mod/mod_fcgid.xml
URL: http://svn.apache.org/viewvc/httpd/mod_fcgid/trunk/docs/manual/mod/mod_fcgid.xml?rev=1311569&r1=1311568&r2=1311569&view=diff
==============================================================================
--- httpd/mod_fcgid/trunk/docs/manual/mod/mod_fcgid.xml (original)
+++ httpd/mod_fcgid/trunk/docs/manual/mod/mod_fcgid.xml Tue Apr 10 04:05:23 2012
@@ -1124,6 +1124,22 @@
</directivesynopsis>
<directivesynopsis>
+ <name>FcgidWin32PreventOrphans</name>
+ <description>Job Control orphan prevention for fcgi workers.</description>
+ <syntax>FcgidWin32PreventOrphans</syntax>
+ <default>[disabled]</default>
+ <contextlist><context>server config</context></contextlist>
+ <usage>
+ <p>Uses Job Control Objects on Windows, only, to enforce shutdown of all
+ fcgi processes created by the httpd worker when the httpd worker has been
+ terminated. Processes terminated in this way do not have the opportunity
+ to clean up gracefully, complete pending disk writes, or similar closure
+ transactions, therefore this behavior is experimental and disabled, by
+ default.</p>
+ </usage>
+ </directivesynopsis>
+
+ <directivesynopsis>
<name>FcgidWrapper</name>
<description>The CGI wrapper setting</description>
<syntax>FcgidWrapper <em>command</em> [ <em>suffix</em> ] [ virtual ]</syntax>
Modified: httpd/mod_fcgid/trunk/modules/fcgid/fcgid_conf.c
URL: http://svn.apache.org/viewvc/httpd/mod_fcgid/trunk/modules/fcgid/fcgid_conf.c?rev=1311569&r1=1311568&r2=1311569&view=diff
==============================================================================
--- httpd/mod_fcgid/trunk/modules/fcgid/fcgid_conf.c (original)
+++ httpd/mod_fcgid/trunk/modules/fcgid/fcgid_conf.c Tue Apr 10 04:05:23 2012
@@ -749,6 +749,71 @@ const char *set_access_authoritative(cmd
return NULL;
}
+
+#ifdef WIN32
+/* FcgidWin32PreventOrphans
+ *
+ * When Apache process gets recycled or shutdown abruptly, CGI processes
+ * spawned by mod_fcgid will get orphaned. Orphaning happens mostly when
+ * Apache worker threads take more than 30 seconds to exit gracefully.
+ *
+ * Apache when run as windows service during shutdown/restart of service
+ * process (master/parent) will terminate child httpd process within 30
+ * seconds (refer \server\mpm\winnt\mpm_winnt.c:master_main()
+ * int timeout = 30000; ~line#1142), therefore if Apache worker threads
+ * are too busy to react to Master's graceful exit signal within 30 seconds
+ * mod_fcgid cleanup routines will not get invoked (refer child_main()
+ * \server\mpm\winnt\child.c: apr_pool_destroy(pchild); ~line#2275)
+ * thereby orphaning all mod_fcgid spwaned CGI processes. Therefore we utilize
+ * Win32 JobObjects to clean up child processes automatically so that CGI
+ * processes are force-killed by win32 during abnormal mod_fcgid termination.
+ *
+ */
+const char *set_win32_prevent_process_orphans(cmd_parms *cmd, void *dummy,
+ char *arg)
+{
+ server_rec *s = cmd->server;
+ fcgid_server_conf *config = ap_get_module_config(s->module_config,
+ &fcgid_module);
+
+ if (config != NULL && config->hJobObjectForAutoCleanup == NULL)
+ {
+ /* Create Win32 job object to prevent CGI process oprhaning
+ */
+ JOBOBJECT_EXTENDED_LIMIT_INFORMATION job_info = { 0 };
+ config->hJobObjectForAutoCleanup = CreateJobObject(NULL, NULL);
+
+ if (config->hJobObjectForAutoCleanup == NULL) {
+ ap_log_error(APLOG_MARK, APLOG_ERR, apr_get_os_error(), NULL,
+ "mod_fcgid: Error enabling CGI process orphan "
+ "prevention: unable to create job object.");
+ return NULL;
+ }
+
+ /* Set job info so that all spawned CGI processes are associated
+ * with mod_fcgid
+ */
+ job_info.BasicLimitInformation.LimitFlags
+ = JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE;
+ if (SetInformationJobObject(config->hJobObjectForAutoCleanup,
+ JobObjectExtendedLimitInformation,
+ &job_info, sizeof(job_info)) == 0) {
+ ap_log_error(APLOG_MARK, APLOG_ERR, apr_get_os_error(), NULL,
+ "mod_fcgid: Error enabling CGI process orphan "
+ "prevention: unable to set job object information.");
+ CloseHandle(config->hJobObjectForAutoCleanup);
+ config->hJobObjectForAutoCleanup = NULL;
+ return NULL;
+ }
+
+ ap_log_error(APLOG_MARK, APLOG_INFO, 0, NULL,
+ "mod_fcgid: Enabled CGI process orphan prevention flag.");
+ }
+
+ return NULL;
+}
+#endif /* WIN32*/
+
fcgid_cmd_conf *get_access_info(request_rec * r, int *authoritative)
{
fcgid_dir_conf *config =
Modified: httpd/mod_fcgid/trunk/modules/fcgid/fcgid_conf.h
URL: http://svn.apache.org/viewvc/httpd/mod_fcgid/trunk/modules/fcgid/fcgid_conf.h?rev=1311569&r1=1311568&r2=1311569&view=diff
==============================================================================
--- httpd/mod_fcgid/trunk/modules/fcgid/fcgid_conf.h (original)
+++ httpd/mod_fcgid/trunk/modules/fcgid/fcgid_conf.h Tue Apr 10 04:05:23 2012
@@ -71,6 +71,10 @@ typedef struct {
int termination_score;
int time_score;
int zombie_scan_interval;
+#ifdef WIN32
+ /* FcgidWin32PreventOrphans - Win32 CGI processes automatic cleanup */
+ HANDLE hJobObjectForAutoCleanup;
+#endif
/* global or vhost
* scalar values have corresponding _set field to aid merging
*/
Modified: httpd/mod_fcgid/trunk/modules/fcgid/fcgid_pm_win.c
URL: http://svn.apache.org/viewvc/httpd/mod_fcgid/trunk/modules/fcgid/fcgid_pm_win.c?rev=1311569&r1=1311568&r2=1311569&view=diff
==============================================================================
--- httpd/mod_fcgid/trunk/modules/fcgid/fcgid_pm_win.c (original)
+++ httpd/mod_fcgid/trunk/modules/fcgid/fcgid_pm_win.c Tue Apr 10 04:05:23 2012
@@ -264,6 +264,7 @@ int procmgr_must_exit()
apr_status_t procmgr_stop_procmgr(void *server)
{
apr_status_t status;
+ fcgid_server_conf *conf;
/* Tell the world to die */
g_must_exit = 1;
@@ -281,6 +282,14 @@ apr_status_t procmgr_stop_procmgr(void *
}
}
+ /* Cleanup the Job object if present */
+ conf = ap_get_module_config(((server_rec*)server)->module_config,
+ &fcgid_module);
+
+ if (conf != NULL && conf->hJobObjectForAutoCleanup != NULL) {
+ CloseHandle(conf->hJobObjectForAutoCleanup);
+ }
+
if (g_wakeup_thread)
return apr_thread_join(&status, g_wakeup_thread);
Modified: httpd/mod_fcgid/trunk/modules/fcgid/fcgid_proc_win.c
URL: http://svn.apache.org/viewvc/httpd/mod_fcgid/trunk/modules/fcgid/fcgid_proc_win.c?rev=1311569&r1=1311568&r2=1311569&view=diff
==============================================================================
--- httpd/mod_fcgid/trunk/modules/fcgid/fcgid_proc_win.c (original)
+++ httpd/mod_fcgid/trunk/modules/fcgid/fcgid_proc_win.c Tue Apr 10 04:05:23 2012
@@ -63,6 +63,7 @@ apr_status_t proc_spawn_process(const ch
{
HANDLE *finish_event, listen_handle;
SECURITY_ATTRIBUTES SecurityAttributes;
+ fcgid_server_conf *sconf;
apr_procattr_t *proc_attr;
apr_status_t rv;
apr_file_t *file;
@@ -177,9 +178,31 @@ apr_status_t proc_spawn_process(const ch
if (rv != APR_SUCCESS) {
ap_log_error(APLOG_MARK, APLOG_ERR, rv, procinfo->main_server,
"mod_fcgid: can't run %s", wargv[0]);
+ return rv;
}
- return rv;
+ /* FcgidWin32PreventOrphans feature */
+ sconf = ap_get_module_config(procinfo->main_server->module_config,
+ &fcgid_module);
+ if (sconf == NULL) {
+ ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, procinfo->main_server,
+ "mod_fcgid: missing server configuration record");
+ return APR_SUCCESS;
+ }
+
+ if (sconf->hJobObjectForAutoCleanup != NULL)
+ {
+ /* Associate cgi process to current process */
+ if (AssignProcessToJobObject(sconf->hJobObjectForAutoCleanup,
+ procnode->proc_id.hproc) == 0) {
+ ap_log_error(APLOG_MARK, APLOG_WARNING, apr_get_os_error(),
+ procinfo->main_server,
+ "mod_fcgid: unable to assign child process to "
+ "job object");
+ }
+ }
+
+ return APR_SUCCESS;
}
apr_status_t proc_kill_gracefully(fcgid_procnode *procnode,
Modified: httpd/mod_fcgid/trunk/modules/fcgid/mod_fcgid.c
URL: http://svn.apache.org/viewvc/httpd/mod_fcgid/trunk/modules/fcgid/mod_fcgid.c?rev=1311569&r1=1311568&r2=1311569&view=diff
==============================================================================
--- httpd/mod_fcgid/trunk/modules/fcgid/mod_fcgid.c (original)
+++ httpd/mod_fcgid/trunk/modules/fcgid/mod_fcgid.c Tue Apr 10 04:05:23 2012
@@ -802,6 +802,11 @@ fcgid_init(apr_pool_t * config_pool, apr
return APR_SUCCESS;
}
+#ifdef WIN32
+const char *set_win32_prevent_process_orphans(cmd_parms *cmd, void *dummy,
+ char *arg);
+#endif
+
static const command_rec fcgid_cmds[] = {
AP_INIT_TAKE1("FcgidAccessChecker", set_access_info, NULL,
ACCESS_CONF | OR_FILEINFO,
@@ -895,6 +900,12 @@ static const command_rec fcgid_cmds[] =
AP_INIT_TAKE1("FcgidZombieScanInterval", set_zombie_scan_interval, NULL,
RSRC_CONF,
"scan interval for zombie process"),
+#ifdef WIN32
+ AP_INIT_NO_ARGS("FcgidWin32PreventOrphans",
+ set_win32_prevent_process_orphans, NULL, RSRC_CONF,
+ "Prevented fcgi process orphaning during Apache worker "
+ "abrupt shutdowns [see documentation]"),
+#endif
/* The following directives are all deprecated in favor
* of a consistent use of the Fcgid prefix.
Re: svn commit: r1311569 - in /httpd/mod_fcgid/trunk: CHANGES-FCGID
docs/manual/mod/mod_fcgid.xml modules/fcgid/fcgid_conf.c modules/fcgid/fcgid_conf.h
modules/fcgid/fcgid_pm_win.c modules/fcgid/fcgid_proc_win.c modules/fcgid/mod_fcgid.c
Posted by Jeff Trawick <tr...@gmail.com>.
On Wed, Apr 11, 2012 at 12:08 AM, William A. Rowe Jr.
<wr...@rowe-clan.net> wrote:
> On 4/10/2012 8:27 PM, Jeff Trawick wrote:
>> On Tue, Apr 10, 2012 at 11:55 AM, William A. Rowe Jr.
>> <wr...@rowe-clan.net> wrote:
>>> On 4/10/2012 10:31 AM, Jeff Trawick wrote:
>>>> On Tue, Apr 10, 2012 at 12:05 AM, <wr...@apache.org> wrote:
>>>>>
>>>>> + /* Cleanup the Job object if present */
>>>>> + conf = ap_get_module_config(((server_rec*)server)->module_config,
>>>>> + &fcgid_module);
>>>>> +
>>>>> + if (conf != NULL && conf->hJobObjectForAutoCleanup != NULL) {
>>>>> + CloseHandle(conf->hJobObjectForAutoCleanup);
>>>>> + }
>>>>> +
>>>>
>>>> Isn't it more idiomatic to register a cleanup for the job object
>>>> rather than explicitly checking for whether or not it exists in
>>>> different code?
>>>
>>> +1
>>
>> I haven't touched that. After possibly wasting time hacking the error
>> reporting/handling for when the job object is created, I wonder if
>> this is even the right place to create the job object and potentially
>> register a cleanup. Why not in a post-config hook? Also, is this
>> really needed in parent AND child?
>
> The windows logic needs a lot more thought in relation to the parent and
> child, where this pool of fcgid workers is created, how they are released.
>
> But as job objects, they will be gone as the parent dies, so I believe
> the whole theory is fundamentally sound. Cosmetics like this do deserve
> deeper consideration, but I think it's ready for release as is.
Agreed that it doesn't keep it from working and isn't going to hurt
anyone... I'm done with "tweaking" this feature.
Re: svn commit: r1311569 - in /httpd/mod_fcgid/trunk: CHANGES-FCGID
docs/manual/mod/mod_fcgid.xml modules/fcgid/fcgid_conf.c modules/fcgid/fcgid_conf.h
modules/fcgid/fcgid_pm_win.c modules/fcgid/fcgid_proc_win.c modules/fcgid/mod_fcgid.c
Posted by "William A. Rowe Jr." <wr...@rowe-clan.net>.
On 4/10/2012 8:27 PM, Jeff Trawick wrote:
> On Tue, Apr 10, 2012 at 11:55 AM, William A. Rowe Jr.
> <wr...@rowe-clan.net> wrote:
>> On 4/10/2012 10:31 AM, Jeff Trawick wrote:
>>> On Tue, Apr 10, 2012 at 12:05 AM, <wr...@apache.org> wrote:
>>>>
>>>> + /* Cleanup the Job object if present */
>>>> + conf = ap_get_module_config(((server_rec*)server)->module_config,
>>>> + &fcgid_module);
>>>> +
>>>> + if (conf != NULL && conf->hJobObjectForAutoCleanup != NULL) {
>>>> + CloseHandle(conf->hJobObjectForAutoCleanup);
>>>> + }
>>>> +
>>>
>>> Isn't it more idiomatic to register a cleanup for the job object
>>> rather than explicitly checking for whether or not it exists in
>>> different code?
>>
>> +1
>
> I haven't touched that. After possibly wasting time hacking the error
> reporting/handling for when the job object is created, I wonder if
> this is even the right place to create the job object and potentially
> register a cleanup. Why not in a post-config hook? Also, is this
> really needed in parent AND child?
The windows logic needs a lot more thought in relation to the parent and
child, where this pool of fcgid workers is created, how they are released.
But as job objects, they will be gone as the parent dies, so I believe
the whole theory is fundamentally sound. Cosmetics like this do deserve
deeper consideration, but I think it's ready for release as is.
Re: svn commit: r1311569 - in /httpd/mod_fcgid/trunk: CHANGES-FCGID
docs/manual/mod/mod_fcgid.xml modules/fcgid/fcgid_conf.c modules/fcgid/fcgid_conf.h
modules/fcgid/fcgid_pm_win.c modules/fcgid/fcgid_proc_win.c modules/fcgid/mod_fcgid.c
Posted by Jeff Trawick <tr...@gmail.com>.
On Tue, Apr 10, 2012 at 11:55 AM, William A. Rowe Jr.
<wr...@rowe-clan.net> wrote:
> On 4/10/2012 10:31 AM, Jeff Trawick wrote:
>> On Tue, Apr 10, 2012 at 12:05 AM, <wr...@apache.org> wrote:
>>> Author: wrowe
>>> Date: Tue Apr 10 04:05:23 2012
>>> New Revision: 1311569
>>>
>>> URL: http://svn.apache.org/viewvc?rev=1311569&view=rev
>>> Log:
>>> Introduce FcgidWin32PreventOrphans directive on Windows to use OS
>>> Job Control Objects to terminate all running fcgi's when the worker
>>> process has been abruptly terminated.
>>>
>>> [wrowe: I've changed the volume level on some error levels, moved
>>> the rv into the error log schema, and moved all commentary over to
>>> the actual conf assignment function. With the exception of style
>>> cleanup, this remains almost entirely Thangaraj's creation. Also
>>> have added simple docs.]
>>>
>>> PR: 51078
>>> Submitted by: Thangaraj AntonyCrouse <thangaraj gmail.com>
>>
>> Is anyone opposed to changing the directive from _NO_ARGS to _FLAG?
>
> Make it so.
>
>> (a couple of more comments are below)
>>
>> I'm happy to do the tweaks after confirmation.
>
> Thanks!
>
>>>
>>> + /* Cleanup the Job object if present */
>>> + conf = ap_get_module_config(((server_rec*)server)->module_config,
>>> + &fcgid_module);
>>> +
>>> + if (conf != NULL && conf->hJobObjectForAutoCleanup != NULL) {
>>> + CloseHandle(conf->hJobObjectForAutoCleanup);
>>> + }
>>> +
>>
>> Isn't it more idiomatic to register a cleanup for the job object
>> rather than explicitly checking for whether or not it exists in
>> different code?
>
> +1
I haven't touched that. After possibly wasting time hacking the error
reporting/handling for when the job object is created, I wonder if
this is even the right place to create the job object and potentially
register a cleanup. Why not in a post-config hook? Also, is this
really needed in parent AND child?
>
>>> --- httpd/mod_fcgid/trunk/modules/fcgid/fcgid_proc_win.c (original)
>>> +++ httpd/mod_fcgid/trunk/modules/fcgid/fcgid_proc_win.c Tue Apr 10 04:05:23 2012
>>> @@ -63,6 +63,7 @@ apr_status_t proc_spawn_process(const ch
>>> {
>>> HANDLE *finish_event, listen_handle;
>>> SECURITY_ATTRIBUTES SecurityAttributes;
>>> + fcgid_server_conf *sconf;
>>> apr_procattr_t *proc_attr;
>>> apr_status_t rv;
>>> apr_file_t *file;
>>> @@ -177,9 +178,31 @@ apr_status_t proc_spawn_process(const ch
>>> if (rv != APR_SUCCESS) {
>>> ap_log_error(APLOG_MARK, APLOG_ERR, rv, procinfo->main_server,
>>> "mod_fcgid: can't run %s", wargv[0]);
>>> + return rv;
>>> }
>
> We leave the new return rv, above.
>
>>> - return rv;
>>> + /* FcgidWin32PreventOrphans feature */
>>> + sconf = ap_get_module_config(procinfo->main_server->module_config,
>>> + &fcgid_module);
>>> + if (sconf == NULL) {
>>> + ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, procinfo->main_server,
>>> + "mod_fcgid: missing server configuration record");
>>> + return APR_SUCCESS;
>>> + }
>>
>> Let's just crash here if sconf is NULL. Agreed?
>
> +1
>
--
Born in Roswell... married an alien...
Re: svn commit: r1311569 - in /httpd/mod_fcgid/trunk: CHANGES-FCGID
docs/manual/mod/mod_fcgid.xml modules/fcgid/fcgid_conf.c modules/fcgid/fcgid_conf.h
modules/fcgid/fcgid_pm_win.c modules/fcgid/fcgid_proc_win.c modules/fcgid/mod_fcgid.c
Posted by "William A. Rowe Jr." <wr...@rowe-clan.net>.
On 4/10/2012 10:31 AM, Jeff Trawick wrote:
> On Tue, Apr 10, 2012 at 12:05 AM, <wr...@apache.org> wrote:
>> Author: wrowe
>> Date: Tue Apr 10 04:05:23 2012
>> New Revision: 1311569
>>
>> URL: http://svn.apache.org/viewvc?rev=1311569&view=rev
>> Log:
>> Introduce FcgidWin32PreventOrphans directive on Windows to use OS
>> Job Control Objects to terminate all running fcgi's when the worker
>> process has been abruptly terminated.
>>
>> [wrowe: I've changed the volume level on some error levels, moved
>> the rv into the error log schema, and moved all commentary over to
>> the actual conf assignment function. With the exception of style
>> cleanup, this remains almost entirely Thangaraj's creation. Also
>> have added simple docs.]
>>
>> PR: 51078
>> Submitted by: Thangaraj AntonyCrouse <thangaraj gmail.com>
>
> Is anyone opposed to changing the directive from _NO_ARGS to _FLAG?
Make it so.
> (a couple of more comments are below)
>
> I'm happy to do the tweaks after confirmation.
Thanks!
>>
>> + /* Cleanup the Job object if present */
>> + conf = ap_get_module_config(((server_rec*)server)->module_config,
>> + &fcgid_module);
>> +
>> + if (conf != NULL && conf->hJobObjectForAutoCleanup != NULL) {
>> + CloseHandle(conf->hJobObjectForAutoCleanup);
>> + }
>> +
>
> Isn't it more idiomatic to register a cleanup for the job object
> rather than explicitly checking for whether or not it exists in
> different code?
+1
>> --- httpd/mod_fcgid/trunk/modules/fcgid/fcgid_proc_win.c (original)
>> +++ httpd/mod_fcgid/trunk/modules/fcgid/fcgid_proc_win.c Tue Apr 10 04:05:23 2012
>> @@ -63,6 +63,7 @@ apr_status_t proc_spawn_process(const ch
>> {
>> HANDLE *finish_event, listen_handle;
>> SECURITY_ATTRIBUTES SecurityAttributes;
>> + fcgid_server_conf *sconf;
>> apr_procattr_t *proc_attr;
>> apr_status_t rv;
>> apr_file_t *file;
>> @@ -177,9 +178,31 @@ apr_status_t proc_spawn_process(const ch
>> if (rv != APR_SUCCESS) {
>> ap_log_error(APLOG_MARK, APLOG_ERR, rv, procinfo->main_server,
>> "mod_fcgid: can't run %s", wargv[0]);
>> + return rv;
>> }
We leave the new return rv, above.
>> - return rv;
>> + /* FcgidWin32PreventOrphans feature */
>> + sconf = ap_get_module_config(procinfo->main_server->module_config,
>> + &fcgid_module);
>> + if (sconf == NULL) {
>> + ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, procinfo->main_server,
>> + "mod_fcgid: missing server configuration record");
>> + return APR_SUCCESS;
>> + }
>
> Let's just crash here if sconf is NULL. Agreed?
+1
Re: svn commit: r1311569 - in /httpd/mod_fcgid/trunk: CHANGES-FCGID
docs/manual/mod/mod_fcgid.xml modules/fcgid/fcgid_conf.c modules/fcgid/fcgid_conf.h
modules/fcgid/fcgid_pm_win.c modules/fcgid/fcgid_proc_win.c modules/fcgid/mod_fcgid.c
Posted by Jeff Trawick <tr...@gmail.com>.
On Tue, Apr 10, 2012 at 12:05 AM, <wr...@apache.org> wrote:
> Author: wrowe
> Date: Tue Apr 10 04:05:23 2012
> New Revision: 1311569
>
> URL: http://svn.apache.org/viewvc?rev=1311569&view=rev
> Log:
> Introduce FcgidWin32PreventOrphans directive on Windows to use OS
> Job Control Objects to terminate all running fcgi's when the worker
> process has been abruptly terminated.
>
> [wrowe: I've changed the volume level on some error levels, moved
> the rv into the error log schema, and moved all commentary over to
> the actual conf assignment function. With the exception of style
> cleanup, this remains almost entirely Thangaraj's creation. Also
> have added simple docs.]
>
> PR: 51078
> Submitted by: Thangaraj AntonyCrouse <thangaraj gmail.com>
Is anyone opposed to changing the directive from _NO_ARGS to _FLAG?
(a couple of more comments are below)
I'm happy to do the tweaks after confirmation.
> Modified:
> httpd/mod_fcgid/trunk/CHANGES-FCGID
> httpd/mod_fcgid/trunk/docs/manual/mod/mod_fcgid.xml
> httpd/mod_fcgid/trunk/modules/fcgid/fcgid_conf.c
> httpd/mod_fcgid/trunk/modules/fcgid/fcgid_conf.h
> httpd/mod_fcgid/trunk/modules/fcgid/fcgid_pm_win.c
> httpd/mod_fcgid/trunk/modules/fcgid/fcgid_proc_win.c
> httpd/mod_fcgid/trunk/modules/fcgid/mod_fcgid.c
>
> Modified: httpd/mod_fcgid/trunk/CHANGES-FCGID
> URL: http://svn.apache.org/viewvc/httpd/mod_fcgid/trunk/CHANGES-FCGID?rev=1311569&r1=1311568&r2=1311569&view=diff
> ==============================================================================
> --- httpd/mod_fcgid/trunk/CHANGES-FCGID [utf8] (original)
> +++ httpd/mod_fcgid/trunk/CHANGES-FCGID [utf8] Tue Apr 10 04:05:23 2012
> @@ -1,6 +1,11 @@
> -*- coding: utf-8 -*-
> Changes with mod_fcgid 2.3.7
>
> + *) Introduce FcgidWin32PreventOrphans directive on Windows to use OS
> + Job Control Objects to terminate all running fcgi's when the worker
> + process has been abruptly terminated. PR: 51078
> + [Thangaraj AntonyCrouse <thangaraj gmail.com>]
> +
> *) Periodically clean out the brigades which are pulling in the request
> body for handoff to the fcgid child. PR: 51749
> [Dominic Benson <dominic.benson thirdlight.com>]
>
> Modified: httpd/mod_fcgid/trunk/docs/manual/mod/mod_fcgid.xml
> URL: http://svn.apache.org/viewvc/httpd/mod_fcgid/trunk/docs/manual/mod/mod_fcgid.xml?rev=1311569&r1=1311568&r2=1311569&view=diff
> ==============================================================================
> --- httpd/mod_fcgid/trunk/docs/manual/mod/mod_fcgid.xml (original)
> +++ httpd/mod_fcgid/trunk/docs/manual/mod/mod_fcgid.xml Tue Apr 10 04:05:23 2012
> @@ -1124,6 +1124,22 @@
> </directivesynopsis>
>
> <directivesynopsis>
> + <name>FcgidWin32PreventOrphans</name>
> + <description>Job Control orphan prevention for fcgi workers.</description>
> + <syntax>FcgidWin32PreventOrphans</syntax>
> + <default>[disabled]</default>
> + <contextlist><context>server config</context></contextlist>
> + <usage>
> + <p>Uses Job Control Objects on Windows, only, to enforce shutdown of all
> + fcgi processes created by the httpd worker when the httpd worker has been
> + terminated. Processes terminated in this way do not have the opportunity
> + to clean up gracefully, complete pending disk writes, or similar closure
> + transactions, therefore this behavior is experimental and disabled, by
> + default.</p>
> + </usage>
> + </directivesynopsis>
> +
> + <directivesynopsis>
> <name>FcgidWrapper</name>
> <description>The CGI wrapper setting</description>
> <syntax>FcgidWrapper <em>command</em> [ <em>suffix</em> ] [ virtual ]</syntax>
>
> Modified: httpd/mod_fcgid/trunk/modules/fcgid/fcgid_conf.c
> URL: http://svn.apache.org/viewvc/httpd/mod_fcgid/trunk/modules/fcgid/fcgid_conf.c?rev=1311569&r1=1311568&r2=1311569&view=diff
> ==============================================================================
> --- httpd/mod_fcgid/trunk/modules/fcgid/fcgid_conf.c (original)
> +++ httpd/mod_fcgid/trunk/modules/fcgid/fcgid_conf.c Tue Apr 10 04:05:23 2012
> @@ -749,6 +749,71 @@ const char *set_access_authoritative(cmd
> return NULL;
> }
>
> +
> +#ifdef WIN32
> +/* FcgidWin32PreventOrphans
> + *
> + * When Apache process gets recycled or shutdown abruptly, CGI processes
> + * spawned by mod_fcgid will get orphaned. Orphaning happens mostly when
> + * Apache worker threads take more than 30 seconds to exit gracefully.
> + *
> + * Apache when run as windows service during shutdown/restart of service
> + * process (master/parent) will terminate child httpd process within 30
> + * seconds (refer \server\mpm\winnt\mpm_winnt.c:master_main()
> + * int timeout = 30000; ~line#1142), therefore if Apache worker threads
> + * are too busy to react to Master's graceful exit signal within 30 seconds
> + * mod_fcgid cleanup routines will not get invoked (refer child_main()
> + * \server\mpm\winnt\child.c: apr_pool_destroy(pchild); ~line#2275)
> + * thereby orphaning all mod_fcgid spwaned CGI processes. Therefore we utilize
> + * Win32 JobObjects to clean up child processes automatically so that CGI
> + * processes are force-killed by win32 during abnormal mod_fcgid termination.
> + *
> + */
> +const char *set_win32_prevent_process_orphans(cmd_parms *cmd, void *dummy,
> + char *arg)
> +{
> + server_rec *s = cmd->server;
> + fcgid_server_conf *config = ap_get_module_config(s->module_config,
> + &fcgid_module);
> +
> + if (config != NULL && config->hJobObjectForAutoCleanup == NULL)
> + {
> + /* Create Win32 job object to prevent CGI process oprhaning
> + */
> + JOBOBJECT_EXTENDED_LIMIT_INFORMATION job_info = { 0 };
> + config->hJobObjectForAutoCleanup = CreateJobObject(NULL, NULL);
> +
> + if (config->hJobObjectForAutoCleanup == NULL) {
> + ap_log_error(APLOG_MARK, APLOG_ERR, apr_get_os_error(), NULL,
> + "mod_fcgid: Error enabling CGI process orphan "
> + "prevention: unable to create job object.");
> + return NULL;
> + }
> +
> + /* Set job info so that all spawned CGI processes are associated
> + * with mod_fcgid
> + */
> + job_info.BasicLimitInformation.LimitFlags
> + = JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE;
> + if (SetInformationJobObject(config->hJobObjectForAutoCleanup,
> + JobObjectExtendedLimitInformation,
> + &job_info, sizeof(job_info)) == 0) {
> + ap_log_error(APLOG_MARK, APLOG_ERR, apr_get_os_error(), NULL,
> + "mod_fcgid: Error enabling CGI process orphan "
> + "prevention: unable to set job object information.");
> + CloseHandle(config->hJobObjectForAutoCleanup);
> + config->hJobObjectForAutoCleanup = NULL;
> + return NULL;
> + }
> +
> + ap_log_error(APLOG_MARK, APLOG_INFO, 0, NULL,
> + "mod_fcgid: Enabled CGI process orphan prevention flag.");
> + }
> +
> + return NULL;
> +}
> +#endif /* WIN32*/
> +
> fcgid_cmd_conf *get_access_info(request_rec * r, int *authoritative)
> {
> fcgid_dir_conf *config =
>
> Modified: httpd/mod_fcgid/trunk/modules/fcgid/fcgid_conf.h
> URL: http://svn.apache.org/viewvc/httpd/mod_fcgid/trunk/modules/fcgid/fcgid_conf.h?rev=1311569&r1=1311568&r2=1311569&view=diff
> ==============================================================================
> --- httpd/mod_fcgid/trunk/modules/fcgid/fcgid_conf.h (original)
> +++ httpd/mod_fcgid/trunk/modules/fcgid/fcgid_conf.h Tue Apr 10 04:05:23 2012
> @@ -71,6 +71,10 @@ typedef struct {
> int termination_score;
> int time_score;
> int zombie_scan_interval;
> +#ifdef WIN32
> + /* FcgidWin32PreventOrphans - Win32 CGI processes automatic cleanup */
> + HANDLE hJobObjectForAutoCleanup;
> +#endif
> /* global or vhost
> * scalar values have corresponding _set field to aid merging
> */
>
> Modified: httpd/mod_fcgid/trunk/modules/fcgid/fcgid_pm_win.c
> URL: http://svn.apache.org/viewvc/httpd/mod_fcgid/trunk/modules/fcgid/fcgid_pm_win.c?rev=1311569&r1=1311568&r2=1311569&view=diff
> ==============================================================================
> --- httpd/mod_fcgid/trunk/modules/fcgid/fcgid_pm_win.c (original)
> +++ httpd/mod_fcgid/trunk/modules/fcgid/fcgid_pm_win.c Tue Apr 10 04:05:23 2012
> @@ -264,6 +264,7 @@ int procmgr_must_exit()
> apr_status_t procmgr_stop_procmgr(void *server)
> {
> apr_status_t status;
> + fcgid_server_conf *conf;
>
> /* Tell the world to die */
> g_must_exit = 1;
> @@ -281,6 +282,14 @@ apr_status_t procmgr_stop_procmgr(void *
> }
> }
>
> + /* Cleanup the Job object if present */
> + conf = ap_get_module_config(((server_rec*)server)->module_config,
> + &fcgid_module);
> +
> + if (conf != NULL && conf->hJobObjectForAutoCleanup != NULL) {
> + CloseHandle(conf->hJobObjectForAutoCleanup);
> + }
> +
Isn't it more idiomatic to register a cleanup for the job object
rather than explicitly checking for whether or not it exists in
different code?
> if (g_wakeup_thread)
> return apr_thread_join(&status, g_wakeup_thread);
>
>
> Modified: httpd/mod_fcgid/trunk/modules/fcgid/fcgid_proc_win.c
> URL: http://svn.apache.org/viewvc/httpd/mod_fcgid/trunk/modules/fcgid/fcgid_proc_win.c?rev=1311569&r1=1311568&r2=1311569&view=diff
> ==============================================================================
> --- httpd/mod_fcgid/trunk/modules/fcgid/fcgid_proc_win.c (original)
> +++ httpd/mod_fcgid/trunk/modules/fcgid/fcgid_proc_win.c Tue Apr 10 04:05:23 2012
> @@ -63,6 +63,7 @@ apr_status_t proc_spawn_process(const ch
> {
> HANDLE *finish_event, listen_handle;
> SECURITY_ATTRIBUTES SecurityAttributes;
> + fcgid_server_conf *sconf;
> apr_procattr_t *proc_attr;
> apr_status_t rv;
> apr_file_t *file;
> @@ -177,9 +178,31 @@ apr_status_t proc_spawn_process(const ch
> if (rv != APR_SUCCESS) {
> ap_log_error(APLOG_MARK, APLOG_ERR, rv, procinfo->main_server,
> "mod_fcgid: can't run %s", wargv[0]);
> + return rv;
> }
>
> - return rv;
> + /* FcgidWin32PreventOrphans feature */
> + sconf = ap_get_module_config(procinfo->main_server->module_config,
> + &fcgid_module);
> + if (sconf == NULL) {
> + ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, procinfo->main_server,
> + "mod_fcgid: missing server configuration record");
> + return APR_SUCCESS;
> + }
Let's just crash here if sconf is NULL. Agreed?
> +
> + if (sconf->hJobObjectForAutoCleanup != NULL)
> + {
> + /* Associate cgi process to current process */
> + if (AssignProcessToJobObject(sconf->hJobObjectForAutoCleanup,
> + procnode->proc_id.hproc) == 0) {
> + ap_log_error(APLOG_MARK, APLOG_WARNING, apr_get_os_error(),
> + procinfo->main_server,
> + "mod_fcgid: unable to assign child process to "
> + "job object");
> + }
> + }
> +
> + return APR_SUCCESS;
> }
>
> apr_status_t proc_kill_gracefully(fcgid_procnode *procnode,
>
> Modified: httpd/mod_fcgid/trunk/modules/fcgid/mod_fcgid.c
> URL: http://svn.apache.org/viewvc/httpd/mod_fcgid/trunk/modules/fcgid/mod_fcgid.c?rev=1311569&r1=1311568&r2=1311569&view=diff
> ==============================================================================
> --- httpd/mod_fcgid/trunk/modules/fcgid/mod_fcgid.c (original)
> +++ httpd/mod_fcgid/trunk/modules/fcgid/mod_fcgid.c Tue Apr 10 04:05:23 2012
> @@ -802,6 +802,11 @@ fcgid_init(apr_pool_t * config_pool, apr
> return APR_SUCCESS;
> }
>
> +#ifdef WIN32
> +const char *set_win32_prevent_process_orphans(cmd_parms *cmd, void *dummy,
> + char *arg);
> +#endif
> +
> static const command_rec fcgid_cmds[] = {
> AP_INIT_TAKE1("FcgidAccessChecker", set_access_info, NULL,
> ACCESS_CONF | OR_FILEINFO,
> @@ -895,6 +900,12 @@ static const command_rec fcgid_cmds[] =
> AP_INIT_TAKE1("FcgidZombieScanInterval", set_zombie_scan_interval, NULL,
> RSRC_CONF,
> "scan interval for zombie process"),
> +#ifdef WIN32
> + AP_INIT_NO_ARGS("FcgidWin32PreventOrphans",
> + set_win32_prevent_process_orphans, NULL, RSRC_CONF,
> + "Prevented fcgi process orphaning during Apache worker "
> + "abrupt shutdowns [see documentation]"),
> +#endif
>
> /* The following directives are all deprecated in favor
> * of a consistent use of the Fcgid prefix.
>
>
--
Born in Roswell... married an alien...