perl-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Philippe M. Chiasson" <go...@ectoplasm.org>
Subject Re: Filter and Handlers should not reset $@ if it was set before entering
Date Mon, 09 Aug 2004 21:29:48 GMT


Stas Bekman wrote:
> Philippe M. Chiasson wrote:
> 
>> From todo/release:
>>
>>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
>>
>>The suggested fix handles this problem by saving/restoring the value of $@
>>in handlers/filters if it was set before entry.
> 
> 
> Great
> 
> 
>>Index: src/modules/perl/modperl_callback.c
>>===================================================================
>>RCS file: /home/cvs/modperl-2.0/src/modules/perl/modperl_callback.c,v
>>retrieving revision 1.75
>>diff -u -I$Id -r1.75 modperl_callback.c
>>--- src/modules/perl/modperl_callback.c	9 Jul 2004 08:01:20 -0000	1.75
>>+++ src/modules/perl/modperl_callback.c	9 Aug 2004 20:49:36 -0000
>>@@ -22,6 +22,15 @@
>>     I32 flags = G_EVAL|G_SCALAR;
>>     dSP;
>>     int count, status = OK;
>>+    SV *errsv = Nullsv;
>>+    
>>+    if (SvTRUE(ERRSV)) { /* Save current value of $@ if set */
>>+        errsv = newSVsv(ERRSV);
>>+        MP_TRACE_h(MP_FUNC, "Saving $@='%s' for %s handler", 
>>+                   SvPVX(errsv), 
>>+                   handler->name
>>+                   );
>>+    }
>> 
>>     if ((status = modperl_handler_resolve(aTHX_ &handler, p, s)) != OK) {
>>         return status;
>>@@ -140,6 +149,13 @@
>>     if (SvTRUE(ERRSV)) {
>>         MP_TRACE_h(MP_FUNC, "$@ = %s", SvPV_nolen(ERRSV));
>>         status = HTTP_INTERNAL_SERVER_ERROR;
>>+    }
>>+    else if (errsv) { /* Restore the initial error we entered with */
>>+         sv_setsv(ERRSV, errsv);
>>+         MP_TRACE_h(MP_FUNC, "Restoring $@='%s' for %s handler", 
>>+                   SvPVX(errsv), 
>>+                   handler->name
>>+                   );
>>     }
> 
> 
> Would it be better to localize $@ instead of copying and restoring it?

I wish we could do that, and it was the first thing I attempted.

The problem is that if we do that, even conditionnaly only when $@ is set
on entry, it means that any _real_ errors triggered during the execution
of the handler (setting $@) will also be lost and replaced with the previous
value for $@.

That's why we need to save $@ if it was set, and only restore if nothing
failed during handler execution, otherwise we squish the new $@.

> 
> 
>>     if (status == HTTP_INTERNAL_SERVER_ERROR) {
>>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	9 Aug 2004 20:49:36 -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;
>>+    #This used to be a bug. Make sure that filters don't reset $@
> 
> 
> drop the 'used to be' thingy, we don't care what it used to be. just leave:

Noted.

>        # test that filters don't reset $@
> :)
> 
> 
>>     eval { i_do_not_exist_really_i_do_not() };
>>     # trigger the filter invocation, before using $@
>>     $r->print("# whatever");
> 
> 

-- 
--------------------------------------------------------------------------------
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

Mime
View raw message