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 Wed, 11 Apr 2012 01:27:16 GMT
On Tue, Apr 10, 2012 at 11:55 AM, William A. Rowe Jr.
<wrowe@rowe-clan.net> wrote:
> On 4/10/2012 10:31 AM, Jeff Trawick wrote:
>> 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?
>
> 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...

Mime
View raw message