httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jeff Trawick <traw...@gmail.com>
Subject 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
Date Tue, 10 Apr 2012 15:31:15 GMT
On Tue, Apr 10, 2012 at 12:05 AM,  <wrowe@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...

Mime
View raw message