On Wed, 15 Oct 2003, Steve Hay wrote:
[ .. ]
> I agree it is undesirable to have A::T depend on W::P::I unless
> necessary, and I think we can get pretty close to what we want without it.
>
> How about the attached patch, which is a slight refinement of Randy's
> first patch. There are three refinements here:
This looks good ....
> 1. I've changed the warning in the "else" part of the "if kill(0, $pid)"
> in stop() to simply say that the server has gone away, which is what
> that would mean -- there is no Windows error to retrieve at that point.
That's true ... I have one suggestion: in this section
====================================================================
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-15 10:41:23.647485900
+0100
@@ -12,6 +12,7 @@
use Apache::TestRequest ();
+ if (WIN32) {
+ require Win32::Process;
+ my $obj = $self->{config}->{win32obj};
+ if (my $pid = $obj ? $obj->GetProcessID : $self->pid) {
+ if (kill(0, $pid)) {
+ Win32::Process::KillProcess($pid, 0);
+ warning "server $self->{name} shutdown";
+ }
+ else {
+ warning "Could not shutdown $self->{name}: " .
+ "server has gone away\n";
+ }
====================================================================
you use the word "shutdown" in the else{} block, which
Apache::TestSmoke would use to signal that the server
has shutdown:
my $stopped_ok = ($log =~ /shutdown/) ? 1 : 0;
unless ($stopped_ok) {
error "Failed to stop server\n $log";
exit 1;
}
Do we want, within Apache::TestSmoke, to differentiate
between a true shutdown and an apparent condition of the
server going away? If it did go away, maybe just print out
a warning, rather than an "exit 1;"?
> 2. I've removed the "require Win32;" lines -- FormatMessage() and
> GetLastError() are both listed as "[CORE]" in the Win32 manpage, meaning
> that "require Win32;" is not required ;)
>
> 3. Most importantly, I've moved the clean-up of stale PID files in
> start() to the top of the sub. In Randy's patch, start() would call
> stop() early which would possibly work on a stale PID file, which is
> what we're trying to prevent, and then clean-up the stale file later
> (too late). We can't reliably do the stop() on Win32 when just starting
> up since we'd *have* to be using an *old* PID file at that point. (Such
> stale files may still exist despite our best efforts to delete them when
> stopping, e.g. if the server *crashed*.) Therefore, start() now doesn't
> bother with a stop() on Win32.
That sounds good ...
>
> There are still potential problems with this -- e.g. if the server
> crashes during a smoke run then the parent smoke process that kicked off
> the server will still attempt to stop it (using t/TEST -stop), and will
> be using the PID file left behind by that crash. However, a long smoke
> run would most likely be run on a machine that is not otherwise in use,
> so the chance of that PID having been re-used is less than usual.
>
> Likewise, if a user interactively runs "t/TEST -stop" themselves then
> the PID file used may be stale. But then you could argue that such
> interactive use suggests the user knows what he is doing. Normally
> people just run "nmake test", "perl t/TEST" or "perl t/SMOKE", or else
> they're delving deeper into something and have a better grasp of what is
> going on.
>
> I think I'm happy enough with this. You could conceivably add some code
> to make use of W::P::I *if it is available* to handle the above problems
> better, but it may not be worth it.
I don't think it's worth it either at this point to put in
the logic to use W::P::I if it's available - I think what
you have is a good compromise. And like you said, a user
who may encounter problems in unusual circumstances not
handled by the above will most likely know what to do to
fix things.
--
best regards,
randy
---------------------------------------------------------------------
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
