On Mon, 13 Oct 2003, Steve Hay wrote:
> Randy Kobes wrote:
[ patch included ]
> >which kills the process (if it's alive - that's what
> >kill(0, $pid) is supposed to check), and if it does so,
> >explicitly unlinks the pid file. It also, when the server
> >starts, unlinks a pid file if that exists, as presumably
> >that's a stale one.
> >
> >
> But kill(0, $pid) doesn't check that the $pid is an Apache process
> started by a previous run of the testsuite. If a previous run fell over
> and left a PID file behind (i.e. before reaching this code that would
> have removed the PID file) then the PID file could contain stale
> information and we shouldn't try to kill it.
>
> I think in the case where the PID was retrieved from the "win32obj" we
> can be sure that the PID is ours to kill (this is the case encountered
> at the end of the test run when we are shutting ourselves down), but the
> other case where the PID was retrieved from $self->pid() (which reads
> from the PID file) is riskier.
>
> That riskier case is encountered at the start of the test run, where
> stop() is called from start(). I see that you've slipped some code into
> start() to delete any stale PID file, but that comes *after* stop() has
> been called, so it's too late. You'd need to move it to *before* the
> stop() call, but then the call to $self->pid() in stop() will always
> find nothing, so why bother with that?
>
> If we can't guarantee that the PID file definitely relates to "this"
> process then we shouldn't be trying to kill that PID in stop(). It
> should only use the "win32obj", which we *can* guarantee gets us "our"
> PID. But then there's nothing left to work with on non-Win32 platforms!...
>
> Perhaps the TestServer object should record its PID in $self on startup,
> and that's the only PID that is ever used to shut anything down?
>
> If a (presumably stale) PID file is encountered on startup then it is
> simply unlinked, like your patch does, but maybe with a warning about
> "Unclean shutdown of previous Apache run" like Apache itself spits out
> when starting up with an old PID file present.
>
> The attached patch is a quick hack at what I'm thinking of here. It
> seems to work OK for me (so far), but is obviously untested on non-Win32.
>
> (I've renamed pid() to pid_from_file(). If you don't want to change the
> API then maybe leave pid() alone and call the new method my_pid() or
> something like that?)
>
> - Steve, wishing he'd never started this one...
:)
That's an important point about not killing a process
that may not be Apache's (which the patch I suggested
may try to do in some circumstances). But to guarantee
that (short of delving into the details of the process
using Win32::Process::Info, which is a non-standard
Win32 module with ActivePerl), what about using
Win32::Process' $ProcessObj->GetProcessID to get the
pid, and only kill that pid? In effect, we don't need
the pid file on Win32, as we have access to the process'
$ProcessObj for the run.
=========================================================
Index: TestServer.pm
===================================================================
RCS file: /home/cvs/httpd-test/perl-framework/Apache-Test/lib/Apache/TestServer.pm,v
retrieving revision 1.67
diff -u -r1.67 TestServer.pm
--- TestServer.pm 10 Oct 2003 04:05:56 -0000 1.67
+++ TestServer.pm 13 Oct 2003 15:52:11 -0000
@@ -314,18 +314,18 @@
my $aborted = shift;
if (Apache::TestConfig::WIN32) {
- if ($self->{config}->{win32obj}) {
- $self->{config}->{win32obj}->Kill(0);
+ require Win32::Process;
+ if (my $obj = $self->{config}->{win32obj}) {
+ if (my $pid = $obj->GetProcessID) {
+ Win32::Process::KillProcess($pid, 0);
+ }
+ else {
+ $obj->Kill(0);
+ }
warning "server $self->{name} shutdown";
- return 1;
}
- else {
- require Win32::Process;
- my $pid = $self->pid;
- Win32::Process::KillProcess($pid, 0);
- warning "server $self->{name} shutdown";
- return 1;
- }
+ unlink $self->pid_file if (-f $self->pid_file);
+ return 1;
}
my $pid = 0;
@@ -437,6 +437,12 @@
my $old_sig;
if (Apache::TestConfig::WIN32) {
+ # check for (and remove) any stale pid file
+ if (-f $self->pid_file) {
+ warn "Removing old PID file -- " .
+ "Unclean shutdown of previous test run?\n";
+ unlink $self->pid_file;
+ }
#make sure only 1 process is started for win32
#else Kill will only shutdown the parent
my $one_process = $self->version_of(\%one_process);
==============================================================
--
best regards,
randy
---------------------------------------------------------------------
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
