Re: t/SMOKE on win32
On Wed, 8 Oct 2003, Stas Bekman wrote:
> Stas Bekman wrote:
> > ... You should be able then to reproduce
> > this outside Apache-Test, just doing:
> >
> > httpd > /dev/null &
> >
> > and then trying to run requests on it, should reproduce the problem.
>
> It's probably should be reproducable by a simple attempt to do:
>
> print STDOUT "It works";
>
> from the startup.pl file. Most likely it won't print
> anything and if you check the return code from print,
> it'll indicate failure.
That's right - I think it was in the way the server was
started in Apache::TestServer. Here's a diff that seems
to work, using IPC::Run3; I've tested it both with t/SMOKE
and t/TEST, on Win32. Note that I left the "print $log;"
statements in there, just to see the progress while
debugging.
==
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.23
diff -u -r1.23 TestSmoke.pm
--- Apache-Test/lib/Apache/TestSmoke.pm 12 Sep 2003 02:47:41 - 1.23
+++ Apache-Test/lib/Apache/TestSmoke.pm 9 Oct 2003 07:21:17 -
@@ -15,6 +15,7 @@
use FindBin;
use POSIX ();
use Symbol ();
+use IPC::Run3;
#use constant DEBUG => 1;
@@ -111,7 +112,7 @@
@{ $self->{tests} } = $self->get_tests($test_opts);
-$self->{base_command} = "./TEST";
+$self->{base_command} = "$^X $FindBin::Bin/TEST";
# options common to all
$self->{base_command} .= " -verbose" if $self->{verbose};
@@ -473,16 +474,10 @@
# start server
{
my $command = $self->{start_command};
-open my $pipe, "$command 2>&1|" or die "cannot fork: $!";
-my $oldfh = select $pipe; $| = 1; select $oldfh;
-# XXX: check startup success?
-my $started_ok = 0;
my $log = '';
-while (my $t = <$pipe>) {
-$started_ok = 1 if $t =~ /started/;
-$log .= $t;
-}
-close $pipe;
+run3 $command, undef, \$log, \$log;
+print $log;
+my $started_ok = ($log =~ /started/) ? 1 : 0;
unless ($started_ok) {
error "failed to start server\n $log";
exit 1;
@@ -507,19 +502,11 @@
my $fill = "." x ($max_len - length $test_name);
$self->{total_tests_run}++;
-open my $pipe, "$command $test 2>&1|" or die "cannot fork: $!";
-my $oldfh = select $pipe; $| = 1; select $oldfh;
-
-my $ok = 0;
+my $test_command = "$command $test";
my $log = '';
-while (<$pipe>) {
-$log .= $_;
-
-$ok = 1 if /All tests successful/;
-}
-# it's normal for $command to exit with a failure status if tests
-# fail, so we don't die/report it
-close $pipe;
+run3 $test_command, undef, \$log, \$log;
+print $log;
+my $ok = ($log =~ /All tests successful/) ? 1 : 0;
my @core_files_msg = $self->Apache::TestRun::scan_core_incremental;
@@ -594,16 +581,10 @@
# stop server
{
my $command = $self->{stop_command};
-open my $pipe, "$command 2>&1|" or die "cannot fork: $!";
-my $oldfh = select $pipe; $| = 1; select $oldfh;
-# XXX: check stopup success?
-my $stopped_ok = 0;
my $log = '';
-while (my $t = <$pipe>) {
-$stopped_ok = 1 if $t =~ /shutdown/;
-$log .= $t;
-}
-close $pipe;
+run3 $command, undef, \$log, \$log;
+print $log;
+my $stopped_ok = ($log =~ /shutdown/) ? 1 : 0;
unless ($stopped_ok) {
error "failed to stop server\n $log";
exit 1;
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.66
diff -u -r1.66 TestServer.pm
--- Apache-Test/lib/Apache/TestServer.pm1 Oct 2003 13:45:23 - 1.66
+++ Apache-Test/lib/Apache/TestServer.pm9 Oct 2003 07:21:17 -
@@ -316,12 +316,14 @@
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;
Win32::Process::KillProcess($pid, 0);
+warning "server $self->{name} shutdown";
return 1;
}
}
@@ -443,7 +445,7 @@
Win32::Process::Create($obj,
$httpd,
"$cmd $one_process",
- 0,
+ 1,
Win32::Process::NORMA
Re: t/SMOKE on win32
Randy Kobes wrote:
That's right - I think it was in the way the server was
started in Apache::TestServer. Here's a diff that seems
to work, using IPC::Run3; I've tested it both with t/SMOKE
and t/TEST, on Win32. Note that I left the "print $log;"
statements in there, just to see the progress while
debugging.
Brilliant!
Your patch below seems to have fixed it all for me. I've just set a
smoke running; I'll let you know how it goes. If all is well, as I'm
sure it will be, then this should definitely be committed.
Well done, Randy!
- Steve
==
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.23
diff -u -r1.23 TestSmoke.pm
--- Apache-Test/lib/Apache/TestSmoke.pm 12 Sep 2003 02:47:41 - 1.23
+++ Apache-Test/lib/Apache/TestSmoke.pm 9 Oct 2003 07:21:17 -
@@ -15,6 +15,7 @@
use FindBin;
use POSIX ();
use Symbol ();
+use IPC::Run3;
#use constant DEBUG => 1;
@@ -111,7 +112,7 @@
@{ $self->{tests} } = $self->get_tests($test_opts);
-$self->{base_command} = "./TEST";
+$self->{base_command} = "$^X $FindBin::Bin/TEST";
# options common to all
$self->{base_command} .= " -verbose" if $self->{verbose};
@@ -473,16 +474,10 @@
# start server
{
my $command = $self->{start_command};
-open my $pipe, "$command 2>&1|" or die "cannot fork: $!";
-my $oldfh = select $pipe; $| = 1; select $oldfh;
-# XXX: check startup success?
-my $started_ok = 0;
my $log = '';
-while (my $t = <$pipe>) {
-$started_ok = 1 if $t =~ /started/;
-$log .= $t;
-}
-close $pipe;
+run3 $command, undef, \$log, \$log;
+print $log;
+my $started_ok = ($log =~ /started/) ? 1 : 0;
unless ($started_ok) {
error "failed to start server\n $log";
exit 1;
@@ -507,19 +502,11 @@
my $fill = "." x ($max_len - length $test_name);
$self->{total_tests_run}++;
-open my $pipe, "$command $test 2>&1|" or die "cannot fork: $!";
-my $oldfh = select $pipe; $| = 1; select $oldfh;
-
-my $ok = 0;
+my $test_command = "$command $test";
my $log = '';
-while (<$pipe>) {
-$log .= $_;
-
-$ok = 1 if /All tests successful/;
-}
-# it's normal for $command to exit with a failure status if tests
-# fail, so we don't die/report it
-close $pipe;
+run3 $test_command, undef, \$log, \$log;
+print $log;
+my $ok = ($log =~ /All tests successful/) ? 1 : 0;
my @core_files_msg = $self->Apache::TestRun::scan_core_incremental;
@@ -594,16 +581,10 @@
# stop server
{
my $command = $self->{stop_command};
-open my $pipe, "$command 2>&1|" or die "cannot fork: $!";
-my $oldfh = select $pipe; $| = 1; select $oldfh;
-# XXX: check stopup success?
-my $stopped_ok = 0;
my $log = '';
-while (my $t = <$pipe>) {
-$stopped_ok = 1 if $t =~ /shutdown/;
-$log .= $t;
-}
-close $pipe;
+run3 $command, undef, \$log, \$log;
+print $log;
+my $stopped_ok = ($log =~ /shutdown/) ? 1 : 0;
unless ($stopped_ok) {
error "failed to stop server\n $log";
exit 1;
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.66
diff -u -r1.66 TestServer.pm
--- Apache-Test/lib/Apache/TestServer.pm1 Oct 2003 13:45:23 - 1.66
+++ Apache-Test/lib/Apache/TestServer.pm9 Oct 2003 07:21:17 -
@@ -316,12 +316,14 @@
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;
Win32::Process::KillProcess($pid, 0);
+warning "server $self->{name} shutdown";
return 1;
}
}
@@ -443,7 +445,7 @@
Win32::Process::Create($obj,
$httpd,
"$cmd $one_process",
- 0,
+ 1,
Win32::Process::NORMAL_PRIORITY_CLASS(),
'.') || die Win32::Process::ErrorReport();
$config->{win32obj} = $obj;
=
Re: t/SMOKE on win32
Steve Hay wrote: Randy Kobes wrote: That's right - I think it was in the way the server was started in Apache::TestServer. Here's a diff that seems to work, using IPC::Run3; I've tested it both with t/SMOKE and t/TEST, on Win32. Note that I left the "print $log;" statements in there, just to see the progress while debugging. Brilliant! Your patch below seems to have fixed it all for me. I've just set a smoke running; I'll let you know how it goes. If all is well, as I'm sure it will be, then this should definitely be committed. Here's the summary from "perl t/SMOKE -iterations=2 -times=2": = Summary == Completion : Completed Status : +++ OK +++ Tests run: 564 Iterations (random) made : 2 --- Started at: Thu Oct 9 09:36:16 2003 --- --- Ended at: Thu Oct 9 10:41:29 2003 --- Both "nmake test" and "perl t/TEST" still work fine too. - Steve - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Testing and smoking leave a PID file behind
I notice that running tests via nmake test, t/TEST or t/SMOKE leaves a httpd.pid file behind in t/logs after the server has stopped. Is this supposed to happen? I thought Apache normally deleted the httpd.pid file when it is stopped. I also find that if there is such a httpd.pid file in place from some previous run then t/SMOKE generates an error on my Windows machine because it tries to `cat ...` the file to get the PID to kill, and "cat" isn't available on Windows (unless Cygwin tools or similar are in the PATH), e.g.: = C:\Temp\modperl-2.0>perl t/SMOKE modperl\post_utf8 -iterations=1 times=1 *** Using random number seed: 956513680 (autogenerated) 'cat' is not recognized as an internal or external command, operable program or batch file. *** *** [001-00-00] trying all tests 10 times [...] = (t/TEST doesn't do this `cat`, and "nmake test" runs a t/TEST -clean first to delete the entire t/logs folder so it doesn't try a `cat` either.) The attached patch removes the use of "cat" to read any existing httpd.pid file. But I still have a problem with the httpd.pid file being left behind in the first place. Windows OS's are terrible for re-using the same PID's over and over again within a short space of time, so if a PID file has been left behind from an Apache that has long since stopped then we don't want the next t/SMOKE trying to kill that PID because there's a good chance that it now relates to some other process entirely! That PID file definitely needs to be cleaned up properly when the Apache used for the tests shuts down. The kill_proc() function that I've just supplied a patch for should only be trying to kill anything on the rare occasions that an Apache actually is still running! - Steve --- TestSmoke.pm.orig 2003-09-12 03:47:42.0 +0100 +++ TestSmoke.pm2003-10-09 10:25:37.716887100 +0100 @@ -746,8 +746,9 @@ 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 return; return unless $pid; kill SIGINT => $pid; - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: t/SMOKE on win32
Randy Kobes wrote: On Wed, 8 Oct 2003, Stas Bekman wrote: Stas Bekman wrote: ... You should be able then to reproduce this outside Apache-Test, just doing: httpd > /dev/null & and then trying to run requests on it, should reproduce the problem. It's probably should be reproducable by a simple attempt to do: print STDOUT "It works"; from the startup.pl file. Most likely it won't print anything and if you check the return code from print, it'll indicate failure. That's right - I think it was in the way the server was started in Apache::TestServer. Here's a diff that seems to work, using IPC::Run3; I've tested it both with t/SMOKE and t/TEST, on Win32. Note that I left the "print $log;" statements in there, just to see the progress while debugging. works fine on linux, +1 to commit sans 'print $log', and probably it'd be a good idea to add a comment to why: Win32::Process::Create($obj, $httpd, "$cmd $one_process", - 0, + 1, also we should probably add IPC::Run3 to lib/Bundle/Apache2.pm. one remaining problem is that the move to IPC::Run3 introduced: Ctrl-C is no longer trappable (due to its use of 'system') immediately which causes t/SMOKE think that the test has failed, when it didn't. I'll need to look at how to solve this. __ Stas BekmanJAm_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]
Re: t/SMOKE on win32
Steve Hay wrote: Steve Hay wrote: Randy Kobes wrote: That's right - I think it was in the way the server was started in Apache::TestServer. Here's a diff that seems to work, using IPC::Run3; I've tested it both with t/SMOKE and t/TEST, on Win32. Note that I left the "print $log;" statements in there, just to see the progress while debugging. Brilliant! Your patch below seems to have fixed it all for me. I've just set a smoke running; I'll let you know how it goes. If all is well, as I'm sure it will be, then this should definitely be committed. Here's the summary from "perl t/SMOKE -iterations=2 -times=2": = Summary == Completion : Completed Status : +++ OK +++ Tests run: 564 Iterations (random) made : 2 --- Started at: Thu Oct 9 09:36:16 2003 --- --- Ended at: Thu Oct 9 10:41:29 2003 --- Both "nmake test" and "perl t/TEST" still work fine too. Fantastic! Thanks to Steve, Randy and Barrie! I don't believe this long thread is coming to an end ;) Steve, when the next time you feel like bored, we have another long time outstanding problem on win32 to solve. See: http://marc.theaimsgroup.com/?t=10646048621&r=1&w=2 __ Stas BekmanJAm_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]
Re: current_callback return behavior
indeed. can you post the script? I'll take a look at it too and see
if I can figure out a way to reproduce it as well.
package CGItest;
sorry, I wasn't able to reproduce the warning with that script. maybe it's
something else?
no, I was talking about the change I did to fix the undef-on-exit
problem (i.e. replacing POPi with POPs)
ok. I didn't see how to fix the undef warning with POPi, though, since it's
undocumented what POPi returns when the stack is undef, and whether it will
always compare to PL_sv_undef (whose name indicates to me an undef SV, not
an undef IV, and I'm recalling some of stephen clouse's comments that undef
isn't undef for all values of undef). so, using POPs seems reasonable to me :)
at any rate, the attached status.t/status.pm tests work as expected against
modperl_callback.c 1.59 but break with 1.60.
so, also included is a reinsertion of the logic for HTTP guessing in 1.60
(that I removed in 1.61), but with fixes for the pv/iv problem - no
additional logic was added to correct for things like string return values
or non-HTTP return values.
in the end, what the patch does is add some status tests and maintain the
HTTP return code logic we've always had, while fixing the exit() undef errors.
so I think this is what we agreed on, unless I totally misunderstood
something. feel free to interject if it doesn't match what you had in mind.
after we agree on this, we can figure out what behaviors we want from
modperl_callback() concerning strings, non-HTTP codes, etc.
--Geoff
Index: src/modules/perl/modperl_callback.c
===
RCS file: /home/cvspublic/modperl-2.0/src/modules/perl/modperl_callback.c,v
retrieving revision 1.61
diff -u -r1.61 modperl_callback.c
--- src/modules/perl/modperl_callback.c 7 Oct 2003 19:16:13 - 1.61
+++ src/modules/perl/modperl_callback.c 9 Oct 2003 17:32:19 -
@@ -4,6 +4,7 @@
request_rec *r, server_rec *s, AV *args)
{
CV *cv=Nullcv;
+SV *status_sv;
I32 flags = G_EVAL|G_SCALAR;
dSP;
int count, status = OK;
@@ -71,19 +72,47 @@
SPAGAIN;
if (count != 1) {
+/* XXX can this really happen with G_EVAL|G_SCALAR? */
+MP_TRACE_h(MP_FUNC, "callback count not 1 - assuming OK\n");
+
status = OK;
}
else {
-SV* status_sv = POPs;
+status_sv = POPs;
+
+/* cases arranged in order of expected occurence:
+ * integer returns (Apache::OK)
+ * valid void API calls (die or ModPerl::Util::exit)
+ * strings that ought to be treated as integers ("0")
+ */
if (SvIOK(status_sv)) {
-status = (IV)SvIVx(status_sv);
+status = SvIVX(status_sv);
+MP_TRACE_h(MP_FUNC, "callback returned valid integer status %d\n",
+ status);
+
+}
+else if (status_sv == &PL_sv_undef) {
+/* croak sets count to 1 but the stack to undef with G_EVAL|G_SCALAR
+ * if it was an error, it will be caught with ERRSV below */
+MP_TRACE_h(MP_FUNC, "callback returned undef - assuming OK\n");
+
+status = OK;
+}
+else if (SvPOK(status_sv)) {
+status = SvIVx(status_sv);
+MP_TRACE_h(MP_FUNC, "callback returned a string - coerced to integer
status %d\n",
+ status);
}
else {
-/* ModPerl::Util::exit doesn't return an integer value */
+/* XXX yikes! */
+MP_TRACE_h(MP_FUNC, "callback IV and PV status slots empty - assuming
OK?\n");
+
status = OK;
}
-/* assume OK for 200 (HTTP_OK) */
-if ((status == 200)) {
+
+/* assume OK for non-http status codes and for 200 (HTTP_OK) */
+if (((status > 0) && (status < 100)) ||
+(status == 200) || (status > 600)) {
status = OK;
}
}
Index: t/response/TestModperl/current_callback.pm
===
RCS file: /home/cvspublic/modperl-2.0/t/response/TestModperl/current_callback.pm,v
retrieving revision 1.3
diff -u -r1.3 current_callback.pm
--- t/response/TestModperl/current_callback.pm 31 Mar 2003 01:50:52 - 1.3
+++ t/response/TestModperl/current_callback.pm 9 Oct 2003 17:32:20 -
@@ -36,6 +36,8 @@
die "expecting $expected callback, instead got $callback"
unless $callback eq $expected;
#warn "in callback: $callback\n";
+
+return Apache::OK;
}
1;
--- /dev/null 2003-01-30 05:24:37.0 -0500
+++ t/modperl/status.t 2003-10-09 12:57:03.0 -0400
@@ -0,0 +1,142 @@
+use strict;
+use warnings FATAL => 'all';
+
+use Apache::Test;
+use Apache::TestReq
Re: Testing and smoking leave a PID file behind
Steve Hay wrote: I notice that running tests via nmake test, t/TEST or t/SMOKE leaves a httpd.pid file behind in t/logs after the server has stopped. Is this supposed to happen? I thought Apache normally deleted the httpd.pid file when it is stopped. That's not under Apache-Test's control, it's Apache who manages this file, we aren't supposed to touch it. All we do is reading the pid from it. I also find that if there is such a httpd.pid file in place from some previous run then t/SMOKE generates an error on my Windows machine because it tries to `cat ...` the file to get the PID to kill, and "cat" isn't available on Windows (unless Cygwin tools or similar are in the PATH), e.g.: My bad, though if we are alredy returning if the file has failed to open, this line is redundant then, no? return unless -f $file; > -my $pid = `cat $file`; > -chomp $pid; > +open my $fh, '<', $file or return; > +chomp(my $pid = <$fh>); > +close $fh or return; and close should always succeed or die if you choose to check the return code. I can't see why would you want to silently return on failing close. The attached patch removes the use of "cat" to read any existing httpd.pid file. But I still have a problem with the httpd.pid file being left behind in the first place. Windows OS's are terrible for re-using the same PID's over and over again within a short space of time, so if a PID file has been left behind from an Apache that has long since stopped then we don't want the next t/SMOKE trying to kill that PID because there's a good chance that it now relates to some other process entirely! That PID file definitely needs to be cleaned up properly when the Apache used for the tests shuts down. The kill_proc() function that I've just supplied a patch for should only be trying to kill anything on the rare occasions that an Apache actually is still running! Understood. But there could be other Apache versions running, you don't want to kill just any Apache process. Can we take the pid and check whether it's an apache process before trying to kill it? __ Stas BekmanJAm_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]
Re: current_callback return behavior
Geoffrey Young wrote: indeed. can you post the script? I'll take a look at it too and see if I can figure out a way to reproduce it as well. package CGItest; sorry, I wasn't able to reproduce the warning with that script. maybe it's something else? have you reversed the POPs change? Here is the original report: http://marc.theaimsgroup.com/?l=apache-modperl-dev&m=106505218031357&w=2 __ Stas BekmanJAm_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]
Re: current_callback return behavior
have you reversed the POPs change? of course Here is the original report: http://marc.theaimsgroup.com/?l=apache-modperl-dev&m=106505218031357&w=2 yes, you did leave out the configuration information :) it turns out the user told us exactly what the reproducability problem is - PerlSwitches -w - which is not turned on in the test suite. --Geoff - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: current_callback return behavior
Geoffrey Young wrote:
no, I was talking about the change I did to fix the undef-on-exit
problem (i.e. replacing POPi with POPs)
ok. I didn't see how to fix the undef warning with POPi, though, since
it's undocumented what POPi returns when the stack is undef, and whether
it will always compare to PL_sv_undef (whose name indicates to me an
undef SV, not an undef IV, and I'm recalling some of stephen clouse's
comments that undef isn't undef for all values of undef). so, using
POPs seems reasonable to me :)
What I meant is this:
if (count != 1) {
status = OK;
}
else {
SV* status_sv = POPs;
if (status_sv == &PL_sv_undef) {
/* ModPerl::Util::exit returns undef */
status = OK;
}
else {
status = (IV)SvIV(status_sv);
}
/* assume OK for 200 (HTTP_OK) */
if ((status == 200)) {
status = OK;
}
}
I just tested that the undef problem disappears (I can easily reproduce it)
and the rest works just as before.
I'll try again at getting a test reproduce this problem.
__
Stas BekmanJAm_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]
Re: current_callback return behavior
What I meant is this:
if (count != 1) {
status = OK;
}
else {
SV* status_sv = POPs;
if (status_sv == &PL_sv_undef) {
/* ModPerl::Util::exit returns undef */
status = OK;
}
else {
status = (IV)SvIV(status_sv);
I don't think you don't need that cast.
}
/* assume OK for 200 (HTTP_OK) */
if ((status == 200)) {
status = OK;
}
}
ok, so the difference between the approaches is that mine is explicit,
giving us some useful debug messages for coerced strings in case we have
future problems.
I just tested that the undef problem disappears (I can easily reproduce
it) and the rest works just as before.
well, better than before, since the pv/iv issue is removed :)
it doesn't matter to me whether we do it your way or my way, so long as the
string issue is fixed. I happen to prefer more explicit debug messages myself.
I'll try again at getting a test reproduce this problem.
should be just a matter of setting PerlSwitches -w in the test suite (though
who knows what that will start doing :)
--Geoff
-
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
Re: cvs commit: modperl-2.0/t/modperl status.t
here are some tests so that no matter which way the issue is resolved, we at least know things are working properly. --Geoff [EMAIL PROTECTED] wrote: geoff 2003/10/09 11:40:31 Added: t/response/TestModperl status.pm t/modperl status.t Log: add tests for handler return status logic - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: current_callback return behavior
Geoffrey Young wrote: have you reversed the POPs change? of course Here is the original report: http://marc.theaimsgroup.com/?l=apache-modperl-dev&m=106505218031357&w=2 yes, you did leave out the configuration information :) oops, I was sure that what was important is 'use warnings' in the test. but its scope doesn't go beyond that file, so it wasn't catching the warnings in return from the callback. thanks for this heads up. it turns out the user told us exactly what the reproducability problem is - PerlSwitches -w - which is not turned on in the test suite. Yup, exactly. I can now reproduce it in the test suite. Coolio! __ Stas BekmanJAm_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]
Re: current_callback return behavior
Geoffrey Young wrote:
What I meant is this:
if (count != 1) {
status = OK;
}
else {
SV* status_sv = POPs;
if (status_sv == &PL_sv_undef) {
/* ModPerl::Util::exit returns undef */
status = OK;
}
else {
status = (IV)SvIV(status_sv);
I don't think you don't need that cast.
Sure, we don't need that case. It was a leftover from IVX.
ok, so the difference between the approaches is that mine is explicit,
giving us some useful debug messages for coerced strings in case we have
future problems.
I tend to suggest to keep things simple, we can always expand it in the future
if we really encounter this problem. I don't think we ever had such a problem
with mp1.
I just tested that the undef problem disappears (I can easily
reproduce it) and the rest works just as before.
well, better than before, since the pv/iv issue is removed :)
;)
it doesn't matter to me whether we do it your way or my way, so long as
the string issue is fixed. I happen to prefer more explicit debug
messages myself.
I'll try again at getting a test reproduce this problem.
should be just a matter of setting PerlSwitches -w in the test suite
(though who knows what that will start doing :)
Yup, it reveals a few more issues with undef. I'm looking at it.
I'll commit that change a bit later.
Also how about moving 'PerlSwitches -wT' into Apache::Test's autogenerator? Do
you think any module developer will want not to have these turned on by
default? I believe most will simply forget to add these, causing more problems.
__
Stas BekmanJAm_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]
Re: t/SMOKE on win32
On Thu, 9 Oct 2003, Stas Bekman wrote:
> works fine on linux, +1 to commit sans 'print $log', and
> probably it'd be a good idea to add a comment to why:
>
> Win32::Process::Create($obj,
> $httpd,
> "$cmd $one_process",
> - 0,
> + 1,
OK, I'll do that.
> also we should probably add IPC::Run3 to lib/Bundle/Apache2.pm.
OK ... I also seem to remember that it would be better,
within Apache::TestSmoke, to do a
eval {require IPC::Run3; }
and die if not there, rather than
use IPC::Run3;
Should I do that?
> one remaining problem is that the move to IPC::Run3
> introduced: Ctrl-C is no longer trappable (due to its use
> of 'system') immediately which causes t/SMOKE think that
> the test has failed, when it didn't. I'll need to look at
> how to solve this.
If that's a big problem that you'd rather not wait to be
solved, we could write things such that it'd do the pipe
stuff on Unix and use IPC::Run3 on Win32 (or perhaps
Win32::Process - I think Ctrl-C is trappable with it).
This would mean more branches in the code, though ...
--
best regards,
randy
-
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
Re: current_callback return behavior
should be just a matter of setting PerlSwitches -w in the test suite (though who knows what that will start doing :) Yup, it reveals a few more issues with undef. I'm looking at it. I'll commit that change a bit later. ok, I'll leave it to you, then. the only thing I would suggest is to revert to the HTTP-code logic in 1.60 as I did. that way, the tests I checked in all pass. we can look at that as a separate issue (after I finish my slides :) Also how about moving 'PerlSwitches -wT' into Apache::Test's autogenerator? Do you think any module developer will want not to have these turned on by default? I believe most will simply forget to add these, causing more problems. I'd definitely be for -w, since it's just warnings. adding -T might blow up working test suites for people who are doing taint-unfriendly things. if you can prove that there is an easy way for users to disable it once added, though, then I'd be ok with it. --Geoff - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: t/SMOKE on win32
Randy Kobes wrote:
also we should probably add IPC::Run3 to lib/Bundle/Apache2.pm.
OK ... I also seem to remember that it would be better,
within Apache::TestSmoke, to do a
eval {require IPC::Run3; }
and die if not there, rather than
use IPC::Run3;
Should I do that?
You are right, I remember the build failing when I added your patch and I
didn't have IPC::Run3 installed. This was happening because other modules
require TestSmoke. So it's not enough to just replace require with eval {
require}, we need to move it away from the compilation phase. The best
solution I believe would be to just have:
require IPC::Run3;
inside 'sub run_test'. Letting require() fail when t/SMOKE is run is OK.
one remaining problem is that the move to IPC::Run3
introduced: Ctrl-C is no longer trappable (due to its use
of 'system') immediately which causes t/SMOKE think that
the test has failed, when it didn't. I'll need to look at
how to solve this.
If that's a big problem that you'd rather not wait to be
solved, we could write things such that it'd do the pipe
stuff on Unix and use IPC::Run3 on Win32 (or perhaps
Win32::Process - I think Ctrl-C is trappable with it).
This would mean more branches in the code, though ...
No, no, it's not a showstopper. I just mentioned this as something to look at.
Hopefully we will find a nice solution while keeping IPC::Run3 for all.
__
Stas BekmanJAm_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]
Re: current_callback return behavior
Geoffrey Young wrote: should be just a matter of setting PerlSwitches -w in the test suite (though who knows what that will start doing :) Yup, it reveals a few more issues with undef. I'm looking at it. I'll commit that change a bit later. ok, I'll leave it to you, then. I meant the -w change, not the callback changes. the only thing I would suggest is to revert to the HTTP-code logic in 1.60 as I did. that way, the tests I checked in all pass. we can look at that as a separate issue (after I finish my slides :) I'd rather see you commit the callback changes. Also how about moving 'PerlSwitches -wT' into Apache::Test's autogenerator? Do you think any module developer will want not to have these turned on by default? I believe most will simply forget to add these, causing more problems. I'd definitely be for -w, since it's just warnings. adding -T might blow up working test suites for people who are doing taint-unfriendly things. if you can prove that there is an easy way for users to disable it once added, though, then I'd be ok with it. following this line of thought, someone may write warn-unfriendly things, in which case we should neither enforce it. I'll just fix it in extra.conf.in in both test suites. __ Stas BekmanJAm_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]
current_callback return status revisited
After enabling 'PerlSwitches -w' a few new "Use of uninitialized variable"
warnings have popped up. One of them was due to this child_exit() handler:
sub ModPerl::Test::exit_handler {
my($p, $s) = @_;
$s->log->info("Child process pid=$$ is exiting");
}
which didn't return a value. It actually did return a value via the last call
to info(), which according to XS should return XSRETURN_EMPTY, which for some
reason left some SV on the stack, which was recognized by Devel::Peek as Undef
but it wasn't really &PL_sv_undef. Other than checking why XSRETURN_EMPTY
returned a value at all (which is unrelated to mod_perl), it just shows that a
handler can return absolutely anything, when there is no explicit status
return, so any guessing on our part won't be much of help. I think the
generated warning is a good solution, but the problem is that it doesn't show
where does it happen, since we have left the scope of the callback by the time
we retrieve the value off-the stack. Perl_sv_2iv calls:
void
Perl_report_uninit(pTHX)
{
if (PL_op)
Perl_warner(aTHX_ packWARN(WARN_UNINITIALIZED), PL_warn_uninit,
" in ", OP_DESC(PL_op));
else
Perl_warner(aTHX_ packWARN(WARN_UNINITIALIZED), PL_warn_uninit, "", "");
}
and since PL_op is not defined, it just prints a dummy warnings, with no
pointers. If could fake PL_op to point to that handler that was just executed,
it'll will tell the exact location of the warning. So "all" we have to figure
out is how to fake that PL_op thingy...
Another solution would be to rip-off the logic from Perl_sv_2iv (there are 3
places where it calls report_uninit, and provide a custom warning message, but
there is quite a lot of logic to duplicate, which is executed anyway.
Finally we could try to temprorary override the PL_warn_uninit format string
to provide a custom error message. may be that will be the easiest thing to do.
I'll post later if I come up with some nice solution.
__
Stas BekmanJAm_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]
Re: current_callback return status revisited
heh, revistited... it never was about current_callback. my bad :)
Stas Bekman wrote:
After enabling 'PerlSwitches -w' a few new "Use of uninitialized
variable" warnings have popped up. One of them was due to this
child_exit() handler:
sub ModPerl::Test::exit_handler {
my($p, $s) = @_;
$s->log->info("Child process pid=$$ is exiting");
}
which didn't return a value. It actually did return a value via the last
call to info(), which according to XS should return XSRETURN_EMPTY,
which for some reason left some SV on the stack, which was recognized by
Devel::Peek as Undef but it wasn't really &PL_sv_undef.
exactly - undef isn't PL_sv_undef for all definitions of undef, which is
partially why I didn't want to explicity check against PL_sv_undef, but
rather check first the IV slot, then the PV slot, _then_ fallback.
Other than
checking why XSRETURN_EMPTY returned a value at all (which is unrelated
to mod_perl), it just shows that a handler can return absolutely
anything, when there is no explicit status return, so any guessing on
our part won't be much of help.
yup.
I think the generated warning is a good
solution, but the problem is that it doesn't show where does it happen,
since we have left the scope of the callback by the time we retrieve the
value off-the stack. Perl_sv_2iv calls:
void
Perl_report_uninit(pTHX)
{
if (PL_op)
Perl_warner(aTHX_ packWARN(WARN_UNINITIALIZED), PL_warn_uninit,
" in ", OP_DESC(PL_op));
else
Perl_warner(aTHX_ packWARN(WARN_UNINITIALIZED), PL_warn_uninit, "",
"");
}
and since PL_op is not defined, it just prints a dummy warnings, with no
pointers. If could fake PL_op to point to that handler that was just
executed, it'll will tell the exact location of the warning. So "all" we
have to figure out is how to fake that PL_op thingy...
Another solution would be to rip-off the logic from Perl_sv_2iv (there
are 3 places where it calls report_uninit, and provide a custom warning
message, but there is quite a lot of logic to duplicate, which is
executed anyway.
Finally we could try to temprorary override the PL_warn_uninit format
string to provide a custom error message. may be that will be the
easiest thing to do.
uh... sure :)
or we could just check for IV (normal circumstances), PV (coerced ints),
PL_sv_undef (real calls to Perl_croak), then throw an official error_log
message for anything else.
I'm not against putting things in the error_log if it's due to not following
the API, and the API requires that handlers exit with a return code that is
meaningful to Apache - various definitions of "undef" are not meaningful.
so, save the special cases like die and MP::U::exit() where we fix things
(trusting that Perl returns the real PL_sv_undef) just throw the error.
something like "Perl handler did not return a valid value" or something. I
don't see any reason to break that down into the line number - if we can get
the name of the routine from the stash (or anon for a coderef) that ought to
be sufficient.
when I'm continually bothered by that map_to_storage "filename not defined"
error (which is nonsense), I'm less bothered by error messages like these,
which would be rare but helpful to the end-user.
but that's just my thought... knock yourself out if you _want_ to mess with
the op stuff :)
I'll post later if I come up with some nice solution.
looking forward to it :)
--Geoff
-
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
Re: [Patch] Support for argument-less blocks
So, for now, users have to resort to using this syntax (note the extra space): The following patch fixes this little problem: whee! :) --Geoff - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
