From: Peter Rabbitson <ribasushi@cpan.org>
Date: Fri, 12 Feb 2016 08:27:38 +0000 (+0100)
Subject: Make sure handling of exception_action is recursion-safe
X-Git-Url: http://git.shadowcat.co.uk/gitweb/gitweb.cgi?a=commitdiff_plain;h=7704dbc93be76967fd8b051a3f6b1df32a2467a3;hp=5c33c8beee177383b6c7913989b60629783dedf1;p=dbsrgits%2FDBIx-Class.git

Make sure handling of exception_action is recursion-safe

Pointed out by Lukas Mai in the last (fifth) bullet point of
https://github.com/PerlDancer/Dancer2/issues/1125#issuecomment-180326756

In addition add extra testing making sure that we will not inadvertently
silence $SIG{__DIE__} when the error is *not* transient

Hopefully this is the last piece of the "clean transient exceptions" puzzle,
it's already been way too much faffing: 7cb35852, ddcc02d1 and 5c33c8be :(
---

diff --git a/lib/DBIx/Class/Schema.pm b/lib/DBIx/Class/Schema.pm
index 7527ddf..f8179fb 100644
--- a/lib/DBIx/Class/Schema.pm
+++ b/lib/DBIx/Class/Schema.pm
@@ -1088,7 +1088,7 @@ This guard was activated beginning"
       );
     };
 
-    eval {
+    dbic_internal_try {
       # if it throws - good, we'll assign to @args in the end
       # if it doesn't - do different things depending on RV truthiness
       if( $act->(@args) ) {
@@ -1109,14 +1109,13 @@ This guard was activated beginning"
 
       1;
     }
-
-      or
-
-    # We call this to get the necessary warnings emitted and disregard the RV
-    # as it's definitely an exception if we got as far as this do{} block
-    is_exception(
-      $args[0] = $@
-    );
+    catch {
+      # We call this to get the necessary warnings emitted and disregard the RV
+      # as it's definitely an exception if we got as far as this catch{} block
+      is_exception(
+        $args[0] = $_
+      );
+    };
 
     # Done guarding against https://github.com/PerlDancer/Dancer2/issues/1125
     $guard_disarmed = 1;
diff --git a/t/34exception_action.t b/t/34exception_action.t
index d326bf7..aa803eb 100644
--- a/t/34exception_action.t
+++ b/t/34exception_action.t
@@ -6,6 +6,7 @@ use warnings;
 use Test::More;
 use Test::Exception;
 use Test::Warn;
+use Scalar::Util 'weaken';
 
 use DBICTest;
 
@@ -118,4 +119,19 @@ for my $ap (qw(
   } $exp_warn, 'Proper warning on encountered antipattern';
 }
 
+# ensure we do not get into an infloop
+{
+  weaken( my $s = $schema );
+
+  $schema->exception_action(sub{
+    $s->throw_exception(@_)
+  });
+
+  throws_ok {
+    $schema->storage->dbh_do(sub {
+      $_[1]->do('wgwfwfwghawhjsejsethjwetjesjesjsejsetjes')
+    } )
+  } qr/syntax error/i;
+}
+
 done_testing;
diff --git a/t/storage/reconnect.t b/t/storage/reconnect.t
index c19f44f..9c1c564 100644
--- a/t/storage/reconnect.t
+++ b/t/storage/reconnect.t
@@ -16,12 +16,22 @@ my $db_tmp  = "$db_orig.tmp";
 # Set up the "usual" sqlite for DBICTest
 my $schema = DBICTest->init_schema( sqlite_use_file => 1 );
 
-my $exception_action_count;
-$schema->exception_action(sub {
-  $exception_action_count++;
+my $exception_callback_count;
+my $ea = $schema->exception_action(sub {
+  $exception_callback_count++;
   die @_;
 });
 
+
+# No, this is not a great idea.
+# Yes, people do it anyway.
+# Might as well test that we have fixed it for good, by never invoking
+# a potential __DIE__ handler in internal_try() stacks. In cases of regular
+# exceptions we expect *both* the exception action *AND* the __DIE__ to
+# fire once
+$SIG{__DIE__} = sub { &$ea };
+
+
 # Make sure we're connected by doing something
 my @art = $schema->resultset("Artist")->search({ }, { order_by => { -desc => 'name' }});
 cmp_ok(@art, '==', 3, "Three artists returned");
@@ -98,7 +108,7 @@ for my $ctx (keys %$ctx_map) {
 
   # start disconnected and then connected
   $schema->storage->disconnect;
-  $exception_action_count = 0;
+  $exception_callback_count = 0;
 
   for (1, 2) {
     my $disarmed;
@@ -115,7 +125,7 @@ for my $ctx (keys %$ctx_map) {
     }, @$args) });
   }
 
-  is( $exception_action_count, 0, 'exception_action never called' );
+  is( $exception_callback_count, 0, 'neither exception_action nor $SIG{__DIE__} ever called' );
 };
 
 # make sure RT#110429 does not recur on manual DBI-side disconnect
@@ -149,7 +159,7 @@ for my $cref (
   note( "Testing with " . B::Deparse->new->coderef2text($cref) );
 
   $schema->storage->disconnect;
-  $exception_action_count = 0;
+  $exception_callback_count = 0;
 
   ok( !$schema->storage->connected, 'Not connected' );
 
@@ -164,13 +174,13 @@ for my $cref (
 
   is( $schema->storage->transaction_depth, undef, "Depth expectedly unknown after failed rollbacks" );
 
-  is( $exception_action_count, 1, "exception_action called only once" );
+  is( $exception_callback_count, 2, 'exception_action and $SIG{__DIE__} called only once each' );
 }
 
 # check exception_action under tenacious disconnect
 {
   $schema->storage->disconnect;
-  $exception_action_count = 0;
+  $exception_callback_count = 0;
 
   throws_ok { $schema->txn_do(sub {
     $schema->storage->_dbh->disconnect;
@@ -178,7 +188,7 @@ for my $cref (
     $schema->resultset('Artist')->next;
   })} qr/prepare on inactive database handle/;
 
-  is( $exception_action_count, 1, "exception_action called only once" );
+  is( $exception_callback_count, 2, 'exception_action and $SIG{__DIE__} called only once each' );
 }
 
 # check that things aren't crazy with a non-violent disconnect