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: r910705 - in /httpd/httpd/trunk: CHANGES docs/manual/programs/htcacheclean.xml support/htcacheclean.c
Date Wed, 17 Feb 2010 18:54:14 GMT
On Tue, Feb 16, 2010 at 6:08 PM, Graham Leggett <minfrin@sharp.fm> wrote:
> On 17 Feb 2010, at 12:11 AM, Jeff Trawick wrote:
>
>>> +  *) support/htcacheclean: Teach it how to write a pid file (modelled on
>>> +     httpd's writing of a pid file) so that it becomes possible to run
>>> +     more than one instance of htcacheclean on the same machine.
>>> +     [Graham Leggett]
>>
>> Based on my own initial confusion of the "becomes possible" language
>> (which isn't strictly true), I suspect that this feature description
>> would be more readily understood if this stressed the ease of
>> termination by pid file, since the admin/script no longer has to check
>> ps output (which certainly is a bigger benefit when there are multiple
>> instances, which would require looking at the path parameter).
>> Mileage varies, of course.
>
> s/possible/practical/ I think makes more sense.
>
> "killall <processname>" is a really awful way to kill a process :)
>
>>> +    "  -P   Specify PIDFILE as the file to write the pid file to."
>>>     NL
>>
>> s/write the pid file to/write the pid to/
>
> Just fixed this in r910735.
>
>>> +            apr_file_printf(file, "%ld" APR_EOL_STR, (long) mypid);
>>
>> APR_PID_T_FMT and no (long) cast
>
> server/log.c is guilty too, fixed it in r910747.
>
>>> +            apr_file_close(file);
>>> +        }
>>> +        else if (!isdaemon) {
>>> +            apr_file_printf(errfile,
>>> +                    "Could not write the pid file '%s': %s" APR_EOL_STR,
>>> +                    pidfile, apr_strerror(status, errmsg, sizeof
>>> errmsg));
>>> +        }
>>
>> why not just let this be a fatal error?  the user specified an invalid
>> path in all likelihood and needs to fix it
>
> We've daemonised at this point, so we can't fail, as the end user won't know
> it's happened. There is also an established pattern in here that when
> daemonised, it stays silent, which probably isn't a good idea, but would
> need to be fixed separately.
>
> One option is to use syslog, will need to see how httpd does it so the
> Windows side works too.

The daemon-report-pidfile-error situation seems to be easily handled
just by reversing the order of the apr_proc_detach block with the
if-pidfile block.  Then the unusable pidfile path error could be
reported to the console in conjunction with -d just like a bogus cache
path or such errors (which are already fatal).

>
>> Also, I suspect that cleaning up the pid file on exit would be
>> anticipated.
>
> Also fixed, svn is off and on and so will commit when it's back up.
>
> server/log.c is also guilty of the same thing, would it make sense to fix
> both at the same time?

Fix both?  Although httpd has left dangling pid files in weird failure
scenarios for a long time, I wouldn't consider it a feature, so yeah.

At the same time?  irrelevant as far as I'm concerned

Mime
View raw message