Re: Testing and smoking leave a PID file behind

[Steve Hay]

Randy Kobes wrote:

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:
   if (my $pid = $obj->GetProcessID) {
       Win32::Process::KillProcess($pid, 0);
   }
   else {
       $obj->Kill(0);
   }
could even just be:

   $obj->Kill(0);

couldn't it?
   

That's true .... I had the two (KillProcessID 

? You mean GetProcessID() followed by KillProcess() I think.

and Kill) in
there so that a second attempt to kill the process would be
made in case the first attempt failed. 

Your code only makes one attempt at killing the process -- either using 
KillProcess() on the PID, if it was able to get the PID, or else using 
Kill() directly.

I'm not sure if there
are circumstances where the 2nd attempt would succeed when
the 1st attempt failed, but I thought it worth a try ...
Do you know if there'd be any advantage to leaving the two
attempts in there?
I don't know of any advantage in getting the PID and using that in 
KillProcess() rather than just using Kill() directly.

In fact, looking at the XS source code, Kill() just calls 
TerminateProcess() on the object that it already has, whereas 
KillProcess() calls OpenProcess() to get an object and then does a 
TerminateProcess() on that.  The latter seems daft when we already have 
an object, hence I thought just $obj->Kill(0) made more sense.

 

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.
   

I was just concentrating on Win32 for the moment - perhaps
it's better to address other OSs separately ...
I was trying to have all the OS's the same where possible, and only do 
something different on Win32 if that didn't work out.  But I suppose 
using the Win32::Process object rather than the PID file is actually a 
better (i.e. less risky) way to do it, so that justifies making it 
different.

 

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?
   

If we adopt the philosophy on Win32 that the pid file not be
used, and instead let the process object handle everything,
then I'd agree that, on Win32, we just let IPC::Run3 handle
things. So for the kill_proc() method of Apache::TestSmoke
we could just return immediately when it's called on Win32.
OK, I'd agree with that.  We shouldn't run around trying to kill 
potentially the wrong process just because IPC::Run3 left something 
running.  Just get Barrie to fix it instead ;-)

For other OSs there may be advanyages to using your patch of
opening and reading the pid file, rather than using `cat
$file`, as the pid() method of Apache::TestServer does.
Yes, I think using open() rather than `cat` is generally a nicer way to 
read a file :-)

You could also take the view that there is no need for kill_proc() at 
all -- If we're happy to rely on IPC::Run3 to do the business for us on 
Win32, then why not on other OS's too?  But there again, if PIDs are not 
re-used so quickly on other OS's then using the PID file to ensure the 
process is gone is much safer than it is on Win32.

 

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?
   

I got it coming up in the console window if I comment out
the unlinking of $self->pid_file when shutting down the
server, and then running the tests again.
 

I found why I didn't get the message:  start() begins by calling stop(), 
which removes the PID file without a warning, so by the time start() 
checks for any left-over PID file, it has already been deleted!  This 
can be fixed by moving stop()'s PID file deletion inside the case where 
a process has actually been killed (since that it not the case when 
stop() has been called from start().)

I now get the warning message.

For clarity, I've attached a patch against CVS with the current state of 
things.

Unfortunately, it doesn't quite work!  Running t/TEST is fine now, but 
we've broken t/SMOKE!  I find that the message "!!! failed to stop 
server" appears at the end of a smoke run, and indeed the server is left 
running.  The reason is that it uses "t/TEST -stop" to stop the server 
(line 586 or so on TestSmoke.pm), which calls TestServer::stop(), which 
no longer does anything unless it has a Win32::Process object!

Would it be possible to change TestSmoke to create a TestServer object 
and call the start() and stop() methods on it, rather than using "t/TEST 
-start" and "t/TEST -stop", or is that a really stupid question?

- Steve
diff -ruN modperl-2.0.orig/Apache-Test/lib/Apache/TestServer.pm 
modperl-2.0/Apache-Test/lib/Apache/TestServer.pm
--- modperl-2.0.orig/Apache-Test/lib/Apache/TestServer.pm       2003-10-10 
05:05:56.000000000 +0100
+++ modperl-2.0/Apache-Test/lib/Apache/TestServer.pm    2003-10-14 09:45:02.967250100 
+0100
@@ -314,18 +314,13 @@
     my $aborted = shift;
 
     if (Apache::TestConfig::WIN32) {
-        if ($self->{config}->{win32obj}) {
-            $self->{config}->{win32obj}->Kill(0);
-            warning "server $self->{name} shutdown";
-            return 1;
-        }
-        else {
+        if (my $obj = $self->{config}->{win32obj}) {
             require Win32::Process;
-            my $pid = $self->pid;
-            Win32::Process::KillProcess($pid, 0);
+            $obj->Kill(0);
             warning "server $self->{name} shutdown";
-            return 1;
-       }
+            unlink $self->pid_file if (-f $self->pid_file);
+        }
+        return 1;
     }
 
     my $pid = 0;
@@ -437,6 +432,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);
diff -ruN modperl-2.0.orig/Apache-Test/lib/Apache/TestSmoke.pm 
modperl-2.0/Apache-Test/lib/Apache/TestSmoke.pm
--- modperl-2.0.orig/Apache-Test/lib/Apache/TestSmoke.pm        2003-10-10 
05:05:56.000000000 +0100
+++ modperl-2.0/Apache-Test/lib/Apache/TestSmoke.pm     2003-10-14 09:26:06.367408500 
+0100
@@ -719,13 +719,18 @@
 sub kill_proc {
     my($self) = @_;
 
+    # We don't use the PID file on Win32: it may be "stale" and PIDs
+    # get re-used too often for this to be safe. Just hope that
+    # IPC::Run3 has cleaned up properly.
+    return if Apache::TestConfig::WIN32;
+
     # 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;
+    open my $fh, '<', $file or return;
+    chomp(my $pid = <$fh>);
+    close $fh or die "Can't close PID file '$file': $!\n"; 
     return unless $pid;
 
     kill SIGINT => $pid;
---------------------------------------------------------------------
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]