Steve Hay wrote:
Perhaps just slip in "$old_pid = 0;" at the end of the "if (WIN32) { ... }" section to fix this?
Sure, the adjusted patch below does that.
Cool.
However, there is still a bigger problem with this new patch. You've removed the little hack that I'd left in kill_proc() to remove the PID file. As you say above this is because you don't want TestSmoke messing with the PID file at all; instead you want to leave it up to TestServer. Unfortunately it seems that TestServer::stop(), which has the code to remove the PID file, doesn't necessarily get run, so the PID file can still get left behind.
I think, that's pretty simple. Are you sure that SIGINIT handler is run when you hit Ctrl-C? It doesn't for me quite often, which would be one explanation why it doesn't get deleted.
The TestSmoke SIGINT handler definitely always gets run. In fact, depending on exactly when I press Ctrl+C I sometimes get the SIGINT handler installed by TestRun run as well as that installed by TestSmoke!
But since we now don't try to kill the process based on the the pid in that httpd.pid file, we don't really have a problem when httpd.pid doesn't get cleaned. Or am I wrong?
TestServer::stop() *does* still use the PID file to kill Apache when we're running a smoke because the win32obj that it would otherwise prefer to use is not available. So we're relying on the fact that stop() shouldn't be called unless we expect *our* PID file to be in place (hence we removed the stop() call from the beginning of start(), for example).
So we shouldn't have a problem with a PID file left behind, but it still makes me a little nervous and I'd rather it got cleaned up if possible.
I've worked it out now. Don't know why I didn't think of this before. The reason that the Apache.exe has already gone away is, of course, that it received the Ctrl+C itself and has stopped of its own accord!
I'm really confused what's going on here. As a test I set "perl t/SMOKE modperl\post_utf8" running and then pressed Ctrl+C to stop it. That's not an unreasonable thing to do, and I would expect it to clean up the PID file, but it didn't. Well, not always, anyway!
It seems that TestRun::try_exit_opts() is run first and only calls TestServer::stop() if the ping() worked. It seems that sometimes it doesn't, i.e. the server is already stopped. I don't know where it gets stopped in that case.
For example, if I Ctrl+C this:
use IPC::Run3;
$SIG{INT} = sub { print "Parent INT\n"; exit };
run3 $^X . ' -e "$SIG{INT} = sub { print \"Child INT\n\"; exit }; 1 while (1)"';
then the output is:
Child INT Parent INT
i.e. the SIGINT is passed onto the spawned child process which also therefore exits (rather than being orphaned).
So the problem is presumably that Apache2 doesn't clean up its own PID file when when it receives a SIGINT. I thought that it did, actually, but a couple of tests revealed some interesting (mis?)behaviour:
- if I run "C:\apache2\bin\apache.exe" in a shell then two Apache.exe processes are started (parent + 1 child), and when I press Ctrl+C they both stop and the PID file is deleted;
- if I run "C:\apache2\bin\apache.exe -X" then, as expected, only one Apache.exe process is started; pressing Ctrl+C still stops it, but the PID file is now left behind!
I don't know why single-process mode behaves differently w.r.t. Ctrl+C, but this is clearly the problem since TestServer::start() does indeed run Apache in single-process mode on Win32.
Ideally this should be fixed in Apache itself, I think (unless there's a good reason for the different behaviour, which I doubt), but in the meantime we could also commit the attached patch to TestRun to workaround Apache's odd behaviour.
This only leaves the problem of it occasionally picking up the "wrong" PID file:-
Anyway, I thought I'd add some code in try_exit_opts() to remove the PID file, but it still didn't always work. I added more code to print out the PID file that it was deleting and found that sometimes it is the "wrong" one. Here's what I've added in try_exit_opts(), just before the "exit_perl $ok" at the end of that function:
my $t_logs = $self->{test_config}->{vars}->{t_logs}; my $pid_file = catfile $t_logs, "httpd.pid"; open my $fh, ">C:\\Temp\\debug.txt"; print $fh "Removing PID file '$pid_file' ...\n"; close $fh; unlink $pid_file if -e $pid_file;
It seems that whether or not this works depends on exactly when I happen to press Ctrl+C. One time it said
Removing PID file 'C:\Temp\modperl-2.0\t\logs\httpd.pid' ...
and sure enough the PID file was gone, but another time it said
Removing PID file 'C:\Temp\modperl-2.0\docs\devel\core_explained\CVS\t\logs\httpd.pid' ...
and, of course, the main PID file 'C:\Temp\modperl-2.0\t\logs\httpd.pid' was left behind.
Now I haven't got a clue what's going on. Any ideas?
File::Find looks for core dumps after each execution. Perhaps you Ctrl-C'ed it when it was inside docs\devel\core_explained?
Maybe, but...
Perhaps we need to expand vars like $self->{test_config}->{vars}->{t_logs} to a full path, in which case it won't be an issue?
... as you see from the output above the (wrong) PID file that it deleted is fully-pathed anyway!
Also this core files scan, is it relevant at all to WIN32? When you get a coredump, do you get a core file?
No, there's no core files on Win32. "Dr Watson's log files" would be the closest thing I suppose, but you don't really want to go there. Perhaps if we skip the coredump scan on Win32 then the problem of the "wrong" PID file being picked up would go away?
Also I should change full dir scan to just scan of the t/ dir, since that's where it dumps the core (the server starts from that directory), which will make the scan much faster, and will make SMOKE much faster as well.
Sounds like a good idea.
I think I'm happy with the patch below now, plus the TestRun patch attached here and/or a fix to Apache to have it clean up its PID file when it is Ctrl+C'd in single-process mode.
- Steve
Index: Apache-Test/lib/Apache/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
--- Apache-Test/lib/Apache/TestServer.pm 10 Oct 2003 04:05:56 -0000 1.67
+++ Apache-Test/lib/Apache/TestServer.pm 18 Oct 2003 22:53:56 -0000
@@ -12,6 +12,7 @@
use Apache::TestRequest ();
use constant COLOR => Apache::TestConfig::COLOR; +use constant WIN32 => Apache::TestConfig::WIN32;
my $CTRL_M = COLOR ? "\r" : "\n";
@@ -313,19 +314,18 @@ my $self = shift; my $aborted = shift;
- if (Apache::TestConfig::WIN32) { - if ($self->{config}->{win32obj}) { - $self->{config}->{win32obj}->Kill(0); - warning "server $self->{name} shutdown"; - return 1; + if (WIN32) { + require Win32::Process; + my $obj = $self->{config}->{win32obj}; + my $pid = -1; + if ($pid = $obj ? $obj->GetProcessID : $self->pid) { + if (kill(0, $pid)) { + Win32::Process::KillProcess($pid, 0); + warning "server $self->{name} shutdown"; + } } - 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 -e $self->pid_file; + return $pid; }
my $pid = 0; @@ -345,7 +345,10 @@
for (1..6) {
if (! $self->ping) {
- return $pid if $_ == 1;
+ if ($_ == 1) {
+ unlink $self->pid_file if -e $self->pid_file;
+ return $pid;
+ }
last;
}
if ($_ == 1) {
@@ -380,10 +383,12 @@
if (--$tries <= 0) {
error "cannot shutdown server on Port $port, ".
"please shutdown manually";
+ unlink $self->pid_file if -e $self->pid_file;
return -1;
}
}
+ unlink $self->pid_file if -e $self->pid_file; return $pid; }
@@ -415,7 +420,24 @@
sub start { my $self = shift; - my $old_pid = $self->stop; + + my $old_pid = -1; + if (WIN32) { + # Stale PID files (e.g. left behind from a previous test run + # that crashed) cannot be trusted on Windows because PID's are + # re-used too frequently, so just remove it. If there is an old + # server still running then the attempt to start a new one below + # will simply fail because the port will be unavailable. + if (-f $self->pid_file) { + error "Removing old PID file -- " . + "Unclean shutdown of previous test run?\n"; + unlink $self->pid_file; + } + $old_pid = 0; + } + else { + $old_pid = $self->stop; + } my $cmd = $self->start_cmd; my $config = $self->{config}; my $vars = $config->{vars}; @@ -436,7 +458,7 @@ print "$cmd\n"; my $old_sig;
- if (Apache::TestConfig::WIN32) {
+ if (WIN32) {
#make sure only 1 process is started for win32
#else Kill will only shutdown the parent
my $one_process = $self->version_of(\%one_process);
@@ -450,7 +472,11 @@
"$cmd $one_process",
1,
Win32::Process::NORMAL_PRIORITY_CLASS(),
- '.') || die Win32::Process::ErrorReport();
+ '.');
+ unless ($obj) {
+ die "Could not start the server: " .
+ Win32::FormatMessage(Win32::GetLastError());
+ }
$config->{win32obj} = $obj;
}
else {
Index: Apache-Test/lib/Apache/TestSmoke.pm
===================================================================
RCS file: /home/cvs/httpd-test/perl-framework/Apache-Test/lib/Apache/TestSmoke.pm,v
retrieving revision 1.24
diff -u -r1.24 TestSmoke.pm
--- Apache-Test/lib/Apache/TestSmoke.pm 10 Oct 2003 04:05:56 -0000 1.24
+++ Apache-Test/lib/Apache/TestSmoke.pm 18 Oct 2003 22:53:56 -0000
@@ -189,9 +189,6 @@
sub run {
my($self) = shift;
- # make sure that there the server is down - $self->kill_proc(); - $self->Apache::TestRun::warn_core(); local $SIG{INT}; $self->install_sighandlers; @@ -577,18 +574,6 @@ $self->logs_end();
# stop server - { - my $command = $self->{stop_command}; - my $log = ''; - IPC::Run3::run3($command, undef, \$log, \$log); - my $stopped_ok = ($log =~ /shutdown/) ? 1 : 0; - unless ($stopped_ok) { - error "failed to stop server\n $log"; - exit 1; - } - } - - # double check that we killed them all? $self->kill_proc();
if ($self->{bug_mode}) { @@ -719,16 +704,15 @@ sub kill_proc { my($self) = @_;
- # a hack - my $t_logs = $self->{test_config}->{vars}->{t_logs}; - my $file = catfile $t_logs, "httpd.pid"; - return unless -f $file; - - my $pid = `cat $file`; - chomp $pid; - return unless $pid; + my $command = $self->{stop_command}; + my $log = ''; + require IPC::Run3; + IPC::Run3::run3($command, undef, \$log, \$log);
- kill SIGINT => $pid; + my $stopped_ok = ($log =~ /shutdown/) ? 1 : 0; + unless ($stopped_ok) { + error "failed to stop server\n $log"; + } }
sub opt_help {
--- TestRun.pm.orig 2003-10-01 19:32:26.000000000 +0100
+++ TestRun.pm 2003-10-20 10:25:45.995323200 +0100
@@ -427,6 +427,9 @@
}
else {
warning "server $self->{server}->{name} is not running";
+ my $t_logs = $self->{test_config}->{vars}->{t_logs};
+ my $pid_file = catfile $t_logs, "httpd.pid";
+ unlink $pid_file if -e $pid_file;
}
exit_perl $ok;
}--------------------------------------------------------------------- To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
