Steve Hay wrote:
Stas Bekman wrote:
Steve Hay wrote: [...]
I suppose we could have Apache::TestSmoke::run_test() look for the "server is not running" message, and have it just warn about it rather than error and exit, but there's nearly nothing else after that anyway so we don't gain much by not exiting. In fact, if the server is not running at the end of the smoke then something has obviously gone wrong, so I think putting out an error makes sense anway.
right on. How about we delegate it to TestServer
TestServer? - this is all TestSmoke...
I meant that Smoke shouldn't mess at all with httpd.pid file and delegate it to Server. Smoke should just say 'stop' and Server should do the right thing. The updated patch is at the bottom.
OK. I understand now. But I still can't get it working :-( ...
and simply do the same as:
# 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; } }
in sub kill_proc {}. in fact we can replace the above block with the new version of kill_proc, which will contain the above block.
I don't think a second kill_proc is really needed in run_test(), but it is needed in the other places in TestSmoke, so I think it'll just work.
OK, I've done that in the attached patch (same as my last one, plus this change).
Cool.
There are actually only 2 other calls to kill_proc(), besides the one at the end of run_test() -- one at the start of run_test() and one in the SIGINT handler. I've had to remove the kill_proc() call from the start of run_test() because it made "t/SMOKE" fall over at the start complaining that it couldn't stop the server (because the new kill_proc() throws an error() if it couldn't do the shutdown)!
I kept the hack in kill_proc() to get at the PID file so that it can be removed, regardless of whether the shutdown succeeded (much like we've done in TestServer::stop()).
OK, I tried to expand the unlink code to the rest of the OSes, so here your patch with a few small tweaks. Please check that I didn't break anything. Thanks.
You did break one thing for sure: After your patch, the beginning of TestServer::start() looks like this:
===== my $self = shift;
my $old_pid = -1;
if (WIN32) {
[...]
if (-f $self->pid_file) {
error "Removing old PID file -- " .
"Unclean shutdown of previous test run?\n";
unlink $self->pid_file;
}
}
else {
$old_pid = $self->stop;
}[...]
if ($old_pid == -1) {
return 0;
}
=====The initialisation of $old_pid to -1 which you've added at the top there means that start() now never does anything on WIN32 because the WIN32 section never changes $old_pid to anything else, and we always return 0!
Perhaps just slip in "$old_pid = 0;" at the end of the "if (WIN32) { ... }" section to fix this?
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'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. 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?
- 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 16 Oct 2003 22:58:21 -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,23 @@
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; + } + } + else { + $old_pid = $self->stop; + } my $cmd = $self->start_cmd; my $config = $self->{config}; my $vars = $config->{vars}; @@ -436,7 +457,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 +471,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 16 Oct 2003 22:58:22 -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 {
__________________________________________________________________ Stas Bekman JAm_pH ------> Just Another mod_perl Hacker http://stason.org/ mod_perl Guide ---> http://perl.apache.org mailto:[EMAIL PROTECTED] http://use.perl.org http://apacheweek.com http://modperlbook.org http://apache.org http://ticketmaster.com
--------------------------------------------------------------------- To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
--------------------------------------------------------------------- To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
