On Tue, Apr 10, 2012 at 11:55 AM, William A. Rowe Jr.
<[email protected]> wrote:
> On 4/10/2012 10:31 AM, Jeff Trawick wrote:
>> On Tue, Apr 10, 2012 at 12:05 AM, <[email protected]> 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...
