Here comes a revised patch that's cleaner by using macros.
Stas Bekman wrote:
Philippe M. Chiasson wrote:
Stas Bekman wrote:
Yes, this $@ save/restore logic could be added to filters only. But, still,
in the case of a failing response handler & filter handler, you'd lose the
error from the filter.
At least, here is a patch that does the same thing, but only for filters.
The problem with filters is that Apache has no provision for error handling. So may be we should at least Perl_warn it, before restoring ERRSV?
Attached patch moves $@ save/restoring to filter logic only, and does warn
in the case of a filter error detected while there was already another known
error before entering the filter
I guess it works for now. I thought that we could still possibly localize $@ at some point.
In any case it looks good, but I'd like to see the added craft wrapped into 2 macros, since it makes the code less readable in the expanded form. Thanks. Feel free to call those macros in either way you prefer.
Maybe we need to be smarter about this a bit and somehow concatenate the
errors ?
Sounds like a good idea. But may be later.
Something like APR::Error supporting aggregated errors maybe ?
May be, I haven't given it any thought. Trying to juggle too many issue at once...
-- -------------------------------------------------------------------------------- Philippe M. Chiasson m/gozer\@(apache|cpan|ectoplasm)\.org/ GPG KeyID : 88C3A5A5 http://gozer.ectoplasm.org/ F9BF E0C2 480E 7680 1AE5 3631 CB32 A107 88C3A5A5
Index: Changes
===================================================================
RCS file: /home/cvs/modperl-2.0/Changes,v
retrieving revision 1.431
diff -u -I$Id -r1.431 Changes
--- Changes 9 Aug 2004 21:42:34 -0000 1.431
+++ Changes 10 Aug 2004 00:04:49 -0000
@@ -12,6 +12,9 @@
=item 1.99_15-dev
+Filters should not reset $@ if it was already set before
+invocation [Gozer]
+
Apache::compat server_root_relative now correctly handles absolute
paths like ap_server_root_relative does [Gozer]
Index: src/modules/perl/modperl_filter.c
===================================================================
RCS file: /home/cvs/modperl-2.0/src/modules/perl/modperl_filter.c,v
retrieving revision 1.93
diff -u -I$Id -r1.93 modperl_filter.c
--- src/modules/perl/modperl_filter.c 2 Jun 2004 21:35:58 -0000 1.93
+++ src/modules/perl/modperl_filter.c 10 Aug 2004 00:04:49 -0000
@@ -54,6 +54,36 @@
} \
safefree(filter);
+/* Save the value of $@ if it was set */
+#define MP_SAVE_ERRSV(tmpsv) \
+ if (SvTRUE(ERRSV)) { \
+ tmpsv = newSVsv(ERRSV); \
+ MP_TRACE_f(MP_FUNC, MP_FILTER_NAME_FORMAT \
+ "Saving [EMAIL PROTECTED]'%s'", \
+ MP_FILTER_NAME(filter->f), \
+ SvPVX(tmpsv) \
+ ); \
+ }
+
+/* Restore previously saved value of $@, warning if a new error was generated */
+#define MP_RESTORE_ERRSV(tmpsv) \
+ if (tmpsv) { \
+ if (SvTRUE(ERRSV)) { \
+ Perl_warn(aTHX_ "%s", SvPVX(ERRSV)); \
+ MP_TRACE_f(MP_FUNC, MP_FILTER_NAME_FORMAT \
+ "error: %s", \
+ MP_FILTER_NAME(filter->f), \
+ SvPVX(ERRSV) \
+ ); \
+ } \
+ sv_setsv(ERRSV, tmpsv); \
+ MP_TRACE_f(MP_FUNC, MP_FILTER_NAME_FORMAT \
+ "Restoring [EMAIL PROTECTED]'%s'", \
+ MP_FILTER_NAME(filter->f), \
+ SvPVX(tmpsv) \
+ ); \
+ }
+
/* this function is for tracing only, it's not optimized for performance */
static int is_modperl_filter(ap_filter_t *f)
{
@@ -432,6 +462,7 @@
int modperl_run_filter(modperl_filter_t *filter)
{
AV *args = Nullav;
+ SV *errsv = Nullsv;
int status;
modperl_handler_t *handler =
((modperl_filter_ctx_t *)filter->f->ctx)->handler;
@@ -443,6 +474,8 @@
MP_dINTERP_SELECT(r, c, s);
+ MP_SAVE_ERRSV(errsv);
+
modperl_handler_make_args(aTHX_ &args,
"Apache::Filter", filter->f,
"APR::Brigade",
@@ -503,6 +536,8 @@
MP_TRACE_f(MP_FUNC, MP_FILTER_NAME_FORMAT
"return: %d\n", modperl_handler_name(handler), status);
+ MP_RESTORE_ERRSV(errsv);
+
return status;
}
Index: t/filter/TestFilter/out_str_eval.pm
===================================================================
RCS file: /home/cvs/modperl-2.0/t/filter/TestFilter/out_str_eval.pm,v
retrieving revision 1.3
diff -u -I$Id -r1.3 out_str_eval.pm
--- t/filter/TestFilter/out_str_eval.pm 3 Jul 2004 18:45:46 -0000 1.3
+++ t/filter/TestFilter/out_str_eval.pm 10 Aug 2004 00:04:49 -0000
@@ -25,9 +25,8 @@
sub response {
my $r = shift;
- plan $r, tests => 1, todo => [1];
- # XXX: see if we can fix filter handlers to restore the original
- # $@ when the callback completes
+ plan $r, tests => 1;
+ # test that filters don't reset $@
eval { i_do_not_exist_really_i_do_not() };
# trigger the filter invocation, before using $@
$r->print("# whatever");
Index: todo/release
===================================================================
RCS file: /home/cvs/modperl-2.0/todo/release,v
retrieving revision 1.41
diff -u -I$Id -r1.41 release
--- todo/release 9 Aug 2004 21:42:35 -0000 1.41
+++ todo/release 10 Aug 2004 00:04:49 -0000
@@ -8,11 +8,6 @@
http://marc.theaimsgroup.com/?l=apache-modperl-dev&m=108967266419527&w=2
owner: gozer
-* filters reset $@ generated by eval, see if we can fix that. The TODO
- test: TestFilter::out_str_eval presents the case
- The description is here:
- http://marc.theaimsgroup.com/?l=apache-modperl-dev&m=108639632031457&w=2
-
* on windows $pool->clean, followed by $pool->destroy breaks other tests
See test TestAPR::pool
http://marc.theaimsgroup.com/?l=apache-modperl-dev&m=108547894817083&w=2
signature.asc
Description: OpenPGP digital signature
