From: Peter Rabbitson <ribasushi@cpan.org>
Date: Thu, 21 Oct 2010 22:37:06 +0000 (+0200)
Subject: Massive cleanup of transaction handlers
X-Git-Url: http://git.shadowcat.co.uk/gitweb/gitweb.cgi?a=commitdiff_plain;h=7d216b101775c472b012b257d848f50d78b193c8;p=dbsrgits%2FDBIx-Class-Historic.git

Massive cleanup of transaction handlers

Audit the txn_do/scope_guard/txn_begin|txn_commit codepaths to make
sure they can be interchangeably used and still just DTRT.
Write a ton of tests to codify current behavior

In the process revert d8a5a2340b0ac0dbf1c3e7779e7951b1bec2ccd5 which
introduces a regression (now poperly caught by tests)
---

diff --git a/lib/DBIx/Class/Storage/DBI.pm b/lib/DBIx/Class/Storage/DBI.pm
index 6a85b2c..29fc1bf 100644
--- a/lib/DBIx/Class/Storage/DBI.pm
+++ b/lib/DBIx/Class/Storage/DBI.pm
@@ -778,8 +778,6 @@ sub txn_do {
   ref $coderef eq 'CODE' or $self->throw_exception
     ('$coderef must be a CODE reference');
 
-  return $coderef->(@_) if $self->{transaction_depth} && ! $self->auto_savepoint;
-
   local $self->{_in_dbh_do} = 1;
 
   my @result;
@@ -794,6 +792,7 @@ sub txn_do {
 
     try {
       $self->txn_begin;
+      my $txn_start_depth = $self->transaction_depth;
       if($want_array) {
           @result = $coderef->(@$args);
       }
@@ -803,14 +802,22 @@ sub txn_do {
       else {
           $coderef->(@$args);
       }
-      $self->txn_commit;
+
+      my $delta_txn = $txn_start_depth - $self->transaction_depth;
+      if ($delta_txn == 0) {
+        $self->txn_commit;
+      }
+      elsif ($delta_txn != 1) {
+        # an off-by-one would mean we fired a rollback
+        carp "Unexpected reduction of transaction depth by $delta_txn after execution of $coderef";
+      }
     } catch {
       $exception = $_;
     };
 
     if(! defined $exception) { return $want_array ? @result : $result[0] }
 
-    if($tried++ || $self->connected) {
+    if($self->transaction_depth > 1 || $tried++ || $self->connected) {
       my $rollback_exception;
       try { $self->txn_rollback } catch { $rollback_exception = shift };
       if(defined $rollback_exception) {
@@ -1368,9 +1375,8 @@ sub svp_rollback {
 }
 
 sub _svp_generate_name {
-    my ($self) = @_;
-
-    return 'savepoint_'.scalar(@{ $self->{'savepoints'} });
+  my ($self) = @_;
+  return 'savepoint_'.scalar(@{ $self->{'savepoints'} });
 }
 
 sub txn_begin {
@@ -1378,9 +1384,18 @@ sub txn_begin {
 
   # this means we have not yet connected and do not know the AC status
   # (e.g. coderef $dbh)
-  $self->ensure_connected if (! defined $self->_dbh_autocommit);
+  if (! defined $self->_dbh_autocommit) {
+    $self->ensure_connected;
+  }
+  # otherwise re-connect on pid changes, so
+  # that the txn_depth is adjusted properly
+  # the lightweight _get_dbh is good enoug here
+  # (only superficial handle check, no pings)
+  else {
+    $self->_get_dbh;
+  }
 
-  if($self->{transaction_depth} == 0) {
+  if($self->transaction_depth == 0) {
     $self->debugobj->txn_begin()
       if $self->debug;
     $self->_dbh_begin_work;
@@ -1420,6 +1435,9 @@ sub txn_commit {
     $self->svp_release
       if $self->auto_savepoint;
   }
+  else {
+    $self->throw_exception( 'Refusing to commit without a started transaction' );
+  }
 }
 
 sub _dbh_commit {
diff --git a/lib/DBIx/Class/Storage/TxnScopeGuard.pm b/lib/DBIx/Class/Storage/TxnScopeGuard.pm
index 7f93113..f686cce 100644
--- a/lib/DBIx/Class/Storage/TxnScopeGuard.pm
+++ b/lib/DBIx/Class/Storage/TxnScopeGuard.pm
@@ -4,13 +4,16 @@ use strict;
 use warnings;
 use Carp::Clan qw/^DBIx::Class/;
 use Try::Tiny;
+use Scalar::Util qw/weaken/;
 use namespace::clean;
 
 sub new {
   my ($class, $storage) = @_;
 
   $storage->txn_begin;
-  bless [ 0, $storage ], ref $class || $class;
+  my $guard = bless [ 0, $storage, $storage->_dbh ], ref $class || $class;
+  weaken ($guard->[2]);
+  $guard;
 }
 
 sub commit {
@@ -25,6 +28,10 @@ sub DESTROY {
 
   return if $dismiss;
 
+  # if our dbh is not ours anymore, the weakref will go undef
+  $storage->_preserve_foreign_dbh;
+  return unless $_[0]->[2];
+
   my $exception = $@;
 
   {
@@ -34,7 +41,9 @@ sub DESTROY {
       unless $exception;
 
     my $rollback_exception;
-    try { $storage->txn_rollback }
+    # do minimal connectivity check due to weird shit like
+    # https://rt.cpan.org/Public/Bug/Display.html?id=62370
+    try { $storage->_seems_connected && $storage->txn_rollback }
     catch { $rollback_exception = shift };
 
     if (defined $rollback_exception && $rollback_exception !~ /DBIx::Class::Storage::NESTED_ROLLBACK_EXCEPTION/) {
diff --git a/t/50fork.t b/t/50fork.t
index df6957c..8fea72f 100644
--- a/t/50fork.t
+++ b/t/50fork.t
@@ -18,7 +18,7 @@ if($num_children !~ /^[0-9]+$/ || $num_children < 10) {
    $num_children = 10;
 }
 
-plan tests => $num_children + 6;
+plan tests => ($num_children*2) + 6;
 
 use lib qw(t/lib);
 
@@ -76,18 +76,36 @@ while(@pids < $num_children) {
 
     $pid = $$;
 
-    my $child_rs = $schema->resultset('CD')->search({ year => 1901 });
-    my $row = $parent_rs->next;
-    if($row && $row->get_column('artist') =~ /^(?:123|456)$/) {
-        $schema->resultset('CD')->create({ title => "test success $pid", artist => $pid, year => scalar(@pids) });
+    my $work = sub {
+      my $child_rs = $schema->resultset('CD')->search({ year => 1901 });
+      my $row = $parent_rs->next;
+      $schema->resultset('CD')->create({ title => "test success $pid", artist => $pid, year => scalar(@pids) })
+        if($row && $row->get_column('artist') =~ /^(?:123|456)$/);
+    };
+
+    # try with and without transactions
+    if ((@pids % 3) == 1) {
+      my $guard = $schema->txn_scope_guard;
+      $work->();
+      $guard->commit;
     }
+    elsif ((@pids % 3) == 2) {
+      $schema->txn_do ($work);
+    }
+    else {
+      $work->();
+    }
+
     sleep(3);
-    exit;
+    exit 0;
 }
 
 ok(1, "past forking");
 
-waitpid($_,0) for(@pids);
+for (@pids) {
+  waitpid($_,0);
+  ok (! $?, "Child $_ exitted cleanly");
+};
 
 ok(1, "past waiting");
 
diff --git a/t/storage/txn.t b/t/storage/txn.t
index 87d1b45..89dddc5 100644
--- a/t/storage/txn.t
+++ b/t/storage/txn.t
@@ -99,6 +99,145 @@ my $code = sub {
   is( $schema->storage->{transaction_depth}, 0, 'txn depth has been reset');
 }
 
+# test nested txn_begin on fresh connection
+{
+  my $schema = DBICTest->init_schema(sqlite_use_file => 1, no_deploy => 1);
+  $schema->storage->ensure_connected;
+
+  is ($schema->storage->transaction_depth, 0, 'Start outside txn');
+
+  my @pids;
+  for my $action (
+    sub {
+      my $s = shift;
+      die "$$ starts in txn!" if $s->storage->transaction_depth != 0;
+      $s->txn_do ( sub {
+        die "$$ not in txn!" if $s->storage->transaction_depth == 0;
+        $s->storage->dbh->do('SELECT 1') } 
+      );
+      die "$$ did not finish txn!" if $s->storage->transaction_depth != 0;
+    },
+    sub {
+      $_[0]->txn_begin;
+      $_[0]->storage->dbh->do('SELECT 1');
+      $_[0]->txn_commit
+    },
+    sub {
+      my $guard = $_[0]->txn_scope_guard;
+      $_[0]->storage->dbh->do('SELECT 1');
+      $guard->commit
+    },
+  ) {
+    push @pids, fork();
+    die "Unable to fork: $!\n"
+      if ! defined $pids[-1];
+
+    if ($pids[-1]) {
+      next;
+    }
+
+    $action->($schema);
+    exit 0;
+  }
+
+  is ($schema->storage->transaction_depth, 0, 'Parent still outside txn');
+
+  for my $pid (@pids) {
+    waitpid ($pid, 0);
+    ok (! $?, "Child $pid exit ok");
+  }
+}
+
+# Test txn_do/scope_guard with forking: outer txn_do
+{
+  my $schema = DBICTest->init_schema( sqlite_use_file => 1 );
+
+  for my $pass (1..2) {
+
+    # do something trying to destabilize the depth count
+    for (1..2) {
+      eval {
+        my $guard = $schema->txn_scope_guard;
+        $schema->txn_do( sub { die } );
+      };
+      $schema->txn_do( sub {
+        ok ($schema->storage->_dbh->do ('SELECT 1'), "Query after exceptions ok ($_)");
+      });
+    }
+
+    for my $pid ( $schema->txn_do ( sub { _forking_action ($schema) } ) ) {
+      waitpid ($pid, 0);
+      ok (! $?, "Child $pid exit ok (pass $pass)");
+      isa_ok ($schema->resultset ('Artist')->find ({ name => "forking action $pid" }), 'DBIx::Class::Row');
+    }
+  }
+}
+
+# same test with outer guard
+{
+  my $schema = DBICTest->init_schema( sqlite_use_file => 1 );
+
+  for my $pass (1..2) {
+
+    # do something trying to destabilize the depth count
+    for (1..2) {
+      eval {
+        my $guard = $schema->txn_scope_guard;
+        $schema->txn_do( sub { die } );
+      };
+      $schema->txn_do( sub {
+        ok ($schema->storage->_dbh->do ('SELECT 1'), "Query after exceptions ok ($_)");
+      });
+    }
+
+    my @pids;
+    my $guard = $schema->txn_scope_guard;
+    _forking_action ($schema);
+    $guard->commit;
+
+    for my $pid (@pids) {
+      waitpid ($pid, 0);
+      ok (! $?, "Child $pid exit ok (pass $pass)");
+      isa_ok ($schema->resultset ('Artist')->find ({ name => "forking action $pid" }), 'DBIx::Class::Row');
+    }
+  }
+}
+
+sub _forking_action {
+  my $schema = shift;
+
+  my @pids;
+  while (@pids < 5) {
+
+    push @pids, fork();
+    die "Unable to fork: $!\n"
+      if ! defined $pids[-1];
+
+    if ($pids[-1]) {
+      next;
+    }
+
+    if (@pids % 2) {
+      $schema->txn_do (sub {
+        my $depth = $schema->storage->transaction_depth;
+        die "$$(txn_do)unexpected txn depth $depth!" if $depth != 1;
+        $schema->resultset ('Artist')->create ({ name => "forking action $$"});
+      });
+    }
+    else {
+      my $guard = $schema->txn_scope_guard;
+      my $depth = $schema->storage->transaction_depth;
+      die "$$(scope_guard) unexpected txn depth $depth!" if $depth != 1;
+      $schema->resultset ('Artist')->create ({ name => "forking action $$"});
+      $guard->commit;
+    }
+
+    exit 0;
+  }
+
+  return @pids;
+}
+
 my $fail_code = sub {
   my ($artist) = @_;
   $artist->create_related('cds', {
@@ -316,12 +455,20 @@ undef $schema;
 
 # make sure the guard does not eat exceptions
 {
-  my $schema = DBICTest->init_schema();
+  my $schema = DBICTest->init_schema;
+
+  no strict 'refs';
+  no warnings 'redefine';
+  local *{DBIx::Class::Storage::DBI::txn_rollback} = sub { die 'die die my darling' };
+
   throws_ok (sub {
     my $guard = $schema->txn_scope_guard;
     $schema->resultset ('Artist')->create ({ name => 'bohhoo'});
 
-    $schema->storage->disconnect;  # this should freak out the guard rollback
+    # this should freak out the guard rollback
+    # but it won't work because DBD::SQLite is buggy
+    # instead just install a toxic rollback above
+    #$schema->storage->_dbh( $schema->storage->_dbh->clone );
 
     die 'Deliberate exception';
   }, qr/Deliberate exception.+Rollback failed/s);
@@ -331,14 +478,21 @@ undef $schema;
 {
   my $schema = DBICTest->init_schema();
 
-  # something is really confusing Test::Warn here, no time to debug
+  no strict 'refs';
+  no warnings 'redefine';
+  local *{DBIx::Class::Storage::DBI::txn_rollback} = sub { die 'die die my darling' };
+
+#The warn from within a DESTROY callback freaks out Test::Warn, do it old-school
 =begin
   warnings_exist (
     sub {
       my $guard = $schema->txn_scope_guard;
       $schema->resultset ('Artist')->create ({ name => 'bohhoo'});
 
-      $schema->storage->disconnect;  # this should freak out the guard rollback
+      # this should freak out the guard rollback
+      # but it won't work because DBD::SQLite is buggy
+      # instead just install a toxic rollback above
+      #$schema->storage->_dbh( $schema->storage->_dbh->clone );
     },
     [
       qr/A DBIx::Class::Storage::TxnScopeGuard went out of scope without explicit commit or error. Rolling back./,
@@ -348,6 +502,7 @@ undef $schema;
   );
 =cut
 
+# delete this once the above works properly (same test)
   my @want = (
     qr/A DBIx::Class::Storage::TxnScopeGuard went out of scope without explicit commit or error. Rolling back./,
     qr/\*+ ROLLBACK FAILED\!\!\! \*+/,
@@ -365,8 +520,6 @@ undef $schema;
   {
       my $guard = $schema->txn_scope_guard;
       $schema->resultset ('Artist')->create ({ name => 'bohhoo'});
-
-      $schema->storage->disconnect;  # this should freak out the guard rollback
   }
 
   is (@w, 2, 'Both expected warnings found');