On Fri, 10 Oct 2003, Steve Hay wrote:Yes, I think you're right.
[ ... ]
I don't know how to do the "graceful" stop, though. What
we want is a bit like trying TERM then KILL on Unix, but
that doesn't work on Win32 -- see the entry for kill() in
the perlport manpage! Win32::Process doesn't have any
"graceful" kill options either, and the "apache.exe -k
stop" command-line that I mentioned above only relates to
installed Apache2 *services*, not processes started from a
command shell. Any ideas, Randy?
At one point I had played around with using a service to get around this problem - have Apache::Test install a temporary service, then use "Apache -k start" / "Apache -k stop" to start / stop the server. While this does things gracefully, so that the pid file is removed, I think there may be some problems with some users not having the right permissions to install a service.
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.
Like you, I can't see how Win32::Process can gracefully kill a process (one seems to need to terminate a process using WM_CLOSE - I've seen examples of how to do this, but this involves non-standard Win32 modules). What about the following: ============================================================ Index: lib/Apache/TestServer.pm =================================================================== RCS file: /home/cvspublic/httpd-test/perl-framework/Apache-Test/lib/Apache/TestServer.pm,v retrieving revision 1.67 diff -u -r1.67 TestServer.pm --- lib/Apache/TestServer.pm 10 Oct 2003 04:05:56 -0000 1.67 +++ lib/Apache/TestServer.pm 10 Oct 2003 17:06:15 -0000 @@ -314,18 +314,22 @@ my $aborted = shift;
if (Apache::TestConfig::WIN32) { - if ($self->{config}->{win32obj}) { - $self->{config}->{win32obj}->Kill(0); - warning "server $self->{name} shutdown"; - return 1; + my $pid; + if (my $obj = $self->{config}->{win32obj}) { + $pid = $obj->GetProcessID(); } else { + $pid = $self->pid; + } + if (kill(0, $pid)) { require Win32::Process; - my $pid = $self->pid; Win32::Process::KillProcess($pid, 0); warning "server $self->{name} shutdown"; - return 1; - } + if (-f $self->pid_file) { + unlink $self->pid_file; + } + } + return 1; }
my $pid = 0; @@ -437,6 +441,10 @@ my $old_sig;
if (Apache::TestConfig::WIN32) {
+ # make sure no stale pid file is around
+ if (-f $self->pid_file) {
+ 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);
==================================================================
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.
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...
--- TestServer.pm.orig 2003-10-10 05:05:56.000000000 +0100
+++ TestServer.pm 2003-10-13 10:47:37.539868100 +0100
@@ -41,6 +41,7 @@
$self->{mpm} = $self->{config}->httpd_mpm || '';
($self->{rev}) = $self->{version} =~ m:^Apache/(\d)\.:;
$self->{rev} ||= 2;
+ $self->{pid} = 0;
$self;
}
@@ -247,6 +248,11 @@
sub pid {
my $self = shift;
+ return $self->{pid};
+}
+
+sub pid_from_file {
+ my $self = shift;
my $file = $self->pid_file;
my $fh = Symbol::gensym();
open $fh, $file or do {
@@ -314,18 +320,16 @@
my $aborted = shift;
if (Apache::TestConfig::WIN32) {
- if ($self->{config}->{win32obj}) {
- $self->{config}->{win32obj}->Kill(0);
- warning "server $self->{name} shutdown";
- return 1;
- }
- else {
- require Win32::Process;
- my $pid = $self->pid;
+ require Win32::Process;
+ my $pid = $self->pid;
+ if ($pid) {
Win32::Process::KillProcess($pid, 0);
warning "server $self->{name} shutdown";
- return 1;
- }
+ }
+ if (-f $self->pid_file) {
+ unlink $self->pid_file;
+ }
+ return $pid;
}
my $pid = 0;
@@ -437,6 +441,11 @@
my $old_sig;
if (Apache::TestConfig::WIN32) {
+ 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);
@@ -451,7 +460,6 @@
1,
Win32::Process::NORMAL_PRIORITY_CLASS(),
'.') || die Win32::Process::ErrorReport();
- $config->{win32obj} = $obj;
}
else {
$old_sig = $SIG{CHLD};
@@ -486,7 +494,7 @@
}
}
- while ($old_pid and $old_pid == $self->pid) {
+ while ($old_pid and $old_pid == $self->pid_from_file) {
warning "old pid file ($old_pid) still exists";
sleep 1;
}
@@ -507,7 +515,7 @@
? ($preamble, sprintf "%02d:%02d", (gmtime $delta)[1,0])
: '.';
sleep 1;
- if ($self->pid) {
+ if ($self->pid_from_file) {
print $preamble, "ok (waited $delta secs)\n";
last;
}
@@ -521,7 +529,8 @@
# dies
$SIG{CHLD} = $old_sig || 'DEFAULT';
- if (my $pid = $self->pid) {
+ if (my $pid = $self->pid_from_file) {
+ $self->{pid} = $pid;
print "server $self->{name} started\n";
my $vh = $config->{vhosts};--------------------------------------------------------------------- To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
