On Mon, 13 Oct 2003, Steve Hay wrote:It's true that we don't need the PID file on Win32. In fact, we don't even need the PID -- the following chunk of your patch below:
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.
if (my $pid = $obj->GetProcessID) {
Win32::Process::KillProcess($pid, 0);
}
else {
$obj->Kill(0);
}could even just be:
$obj->Kill(0);
couldn't it?
But what about other OS's? Stas said that Linux/Unix (and presumably all other supported OS's too) has the same problem with stale PID files kicking around, so I was trying to do something that addressed that too.
I'm happy with your patch below (perhaps simplified as above if you agree there is no point in the GetProcessID() call) on Win32. I'll let others decide what's best on other platforms.
We also still need a solution to the similar problem in TestSmoke.pm -- I posted a patch that changes a nasty `cat $pidfile` to something nicer, but the PID that is read from the file is then killed with a kill(SIGINT => $pid)! (See TestSmoke::kill_proc.) How can we fix that? There isn't a Win32::Process object available there since we've used IPC::Run3 rather than Win32::Process! Perhaps we should just trust that IPC::Run3 has cleaned up correctly and there is no need to kill anything ourselves?
One other nit: I notice that the "Unclean shutdown" message that we've added doesn't seem to appear anywhere! Neither the console nor the error_log! Where does it go?
- Steve
========================================================= 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); ==============================================================
--------------------------------------------------------------------- To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
