From: Peter Rabbitson <ribasushi@cpan.org>
Date: Tue, 26 Apr 2011 06:38:26 +0000 (+0200)
Subject: Another overhaul of transaction/savepoint handling
X-Git-Tag: v0.08191~16
X-Git-Url: http://git.shadowcat.co.uk/gitweb/gitweb.cgi?a=commitdiff_plain;h=90d7422fc;p=dbsrgits%2FDBIx-Class.git

Another overhaul of transaction/savepoint handling

- Move most of the transaction logic back to ::Storage, only leave
  DBI-specific overrides in ::Storage::DBI
- Fix bug where a nested rollback would destabilize the entire execution
  chain
- Better checks for connectivity/ordering of txn/svp operations
- Make DBIC::Storage::NESTED_ROLLBACK_EXCEPTION a proper exception subclass
- Standardize the txn/svp names - renamed the actual workers
  _dbh_begin_work               to _exec_txn_begin
  _dbh_commit                   to _exec_txn_commit
  _dbh_rollback                 to _exec_txn_rollback
  _svp_[begin|release|rollback] to _exec_svp_[begin|release|rollback]
---

diff --git a/Changes b/Changes
index b99dc97..b4f48a7 100644
--- a/Changes
+++ b/Changes
@@ -56,6 +56,7 @@ Revision history for DBIx::Class
           of SQL::Abstract >= 1.73
         - Fix using \[] literals in the from resultset attribute
         - Fix populate() with \[], arrays (datatype) and other exotic values
+        - Fix handling of rollbacks in nested transactions
         - Fix complex limits (RNO/RowNum/FetchFirst/Top/GenSubq) with
           sub-selects in the selectors list (correlated subqueries)
         - Fix inconsistency between $rs->next with and without HRI when all
diff --git a/lib/DBIx/Class/Storage.pm b/lib/DBIx/Class/Storage.pm
index dcc68bc..251c407 100644
--- a/lib/DBIx/Class/Storage.pm
+++ b/lib/DBIx/Class/Storage.pm
@@ -6,34 +6,25 @@ use warnings;
 use base qw/DBIx::Class/;
 use mro 'c3';
 
-use DBIx::Class::Exception;
-use Scalar::Util 'weaken';
+{
+  package # Hide from PAUSE
+    DBIx::Class::Storage::NESTED_ROLLBACK_EXCEPTION;
+  use base 'DBIx::Class::Exception';
+}
+
+use DBIx::Class::Carp;
+use Scalar::Util qw/blessed weaken/;
 use DBIx::Class::Storage::TxnScopeGuard;
 use Try::Tiny;
 use namespace::clean;
 
-__PACKAGE__->mk_group_accessors('simple' => qw/debug schema/);
-__PACKAGE__->mk_group_accessors('component_class' => 'cursor_class');
+__PACKAGE__->mk_group_accessors(simple => qw/debug schema transaction_depth auto_savepoint savepoints/);
+__PACKAGE__->mk_group_accessors(component_class => 'cursor_class');
 
 __PACKAGE__->cursor_class('DBIx::Class::Cursor');
 
 sub cursor { shift->cursor_class(@_); }
 
-package # Hide from PAUSE
-    DBIx::Class::Storage::NESTED_ROLLBACK_EXCEPTION;
-
-use overload '"' => sub {
-  'DBIx::Class::Storage::NESTED_ROLLBACK_EXCEPTION'
-};
-
-sub new {
-  my $class = shift;
-  my $self = {};
-  return bless $self, $class;
-}
-
-package DBIx::Class::Storage;
-
 =head1 NAME
 
 DBIx::Class::Storage - Generic Storage Handler
@@ -58,8 +49,10 @@ sub new {
 
   $self = ref $self if ref $self;
 
-  my $new = {};
-  bless $new, $self;
+  my $new = bless( {
+    transaction_depth => 0,
+    savepoints => [],
+  }, $self);
 
   $new->set_schema($schema);
   $new->debug(1)
@@ -158,7 +151,7 @@ For example,
   } catch {
     my $error = shift;
     # Transaction failed
-    die "something terrible has happened!"   #
+    die "something terrible has happened!"
       if ($error =~ /Rollback failed/);          # Rollback failed
 
     deal_with_failed_transaction();
@@ -186,49 +179,83 @@ sub txn_do {
   ref $coderef eq 'CODE' or $self->throw_exception
     ('$coderef must be a CODE reference');
 
-  my (@return_values, $return_value);
+  my $abort_txn = sub {
+    my ($self, $exception) = @_;
+
+    my $rollback_exception = try { $self->txn_rollback; undef } catch { shift };
 
-  $self->txn_begin; # If this throws an exception, no rollback is needed
+    if ( $rollback_exception and (
+      ! defined blessed $rollback_exception
+          or
+      ! $rollback_exception->isa('DBIx::Class::Storage::NESTED_ROLLBACK_EXCEPTION')
+    ) ) {
+      $self->throw_exception(
+        "Transaction aborted: ${exception}. "
+        . "Rollback failed: ${rollback_exception}"
+      );
+    }
+    $self->throw_exception($exception);
+  };
 
-  my $wantarray = wantarray; # Need to save this since the context
-                             # inside the try{} block is independent
-                             # of the context that called txn_do()
+  # take a ref instead of a copy, to preserve coderef @_ aliasing semantics
   my $args = \@_;
 
-  try {
+  # do not turn on until a succesful txn_begin
+  my $attempt_commit = 0;
 
-    # Need to differentiate between scalar/list context to allow for
-    # returning a list in scalar context to get the size of the list
-    if ($wantarray) {
-      # list context
-      @return_values = $coderef->(@$args);
-    } elsif (defined $wantarray) {
-      # scalar context
-      $return_value = $coderef->(@$args);
-    } else {
-      # void context
-      $coderef->(@$args);
-    }
-    $self->txn_commit;
+  my $txn_init_depth = $self->transaction_depth;
+
+  try {
+    $self->txn_begin;
+    $attempt_commit = 1;
+    $coderef->(@$args)
   }
   catch {
-    my $error = shift;
+    $attempt_commit = 0;
 
-    try {
-      $self->txn_rollback;
-    } catch {
-      my $exception_class = "DBIx::Class::Storage::NESTED_ROLLBACK_EXCEPTION";
-      $self->throw_exception($error)  # propagate nested rollback
-        if $_ =~ /$exception_class/;
+    # init depth of > 0 implies nesting or non-autocommit (either way no retry)
+    if($txn_init_depth or $self->connected ) {
+      $abort_txn->($self, $_);
+    }
+    else {
+      carp "Retrying txn_do($coderef) after catching disconnected exception: $_"
+        if $ENV{DBIC_STORAGE_RETRY_DEBUG};
 
-      $self->throw_exception(
-        "Transaction aborted: $error. Rollback failed: $_"
-      );
+      $self->_populate_dbh;
+
+      # if txn_depth is > 1 this means something was done to the
+      # original $dbh, otherwise we would not get past the if() above
+      $self->throw_exception(sprintf
+        'Unexpected transaction depth of %d on freshly connected handle',
+        $self->transaction_depth,
+      ) if $self->transaction_depth;
+
+      $self->txn_begin;
+      $attempt_commit = 1;
+
+      try {
+        $coderef->(@$args)
+      }
+      catch {
+        $attempt_commit = 0;
+        $abort_txn->($self, $_)
+      };
+    };
+  }
+  finally {
+    if ($attempt_commit) {
+      my $delta_txn = (1 + $txn_init_depth) - $self->transaction_depth;
+
+      if ($delta_txn) {
+        # a rollback in a top-level txn_do is valid-ish (seen in the wild and our own tests)
+        carp "Unexpected reduction of transaction depth by $delta_txn after execution of $coderef, skipping txn_do's commit"
+          unless $delta_txn == 1 and $self->transaction_depth == 0;
+      }
+      else {
+        $self->txn_commit;
+      }
     }
-    $self->throw_exception($error); # txn failed but rollback succeeded
   };
-
-  return wantarray ? @return_values : $return_value;
 }
 
 =head2 txn_begin
@@ -240,7 +267,20 @@ an entire code block to be executed transactionally.
 
 =cut
 
-sub txn_begin { die "Virtual method!" }
+sub txn_begin {
+  my $self = shift;
+
+  if($self->transaction_depth == 0) {
+    $self->debugobj->txn_begin()
+      if $self->debug;
+    $self->_exec_txn_begin;
+  }
+  elsif ($self->auto_savepoint) {
+    $self->svp_begin;
+  }
+  $self->{transaction_depth}++;
+
+}
 
 =head2 txn_commit
 
@@ -251,7 +291,22 @@ transaction currently in effect (i.e. you called L</txn_begin>).
 
 =cut
 
-sub txn_commit { die "Virtual method!" }
+sub txn_commit {
+  my $self = shift;
+
+  if ($self->transaction_depth == 1) {
+    $self->debugobj->txn_commit() if $self->debug;
+    $self->_exec_txn_commit;
+    $self->{transaction_depth}--;
+  }
+  elsif($self->transaction_depth > 1) {
+    $self->{transaction_depth}--;
+    $self->svp_release if $self->auto_savepoint;
+  }
+  else {
+    $self->throw_exception( 'Refusing to commit without a started transaction' );
+  }
+}
 
 =head2 txn_rollback
 
@@ -261,7 +316,31 @@ which allows the rollback to propagate to the outermost transaction.
 
 =cut
 
-sub txn_rollback { die "Virtual method!" }
+sub txn_rollback {
+  my $self = shift;
+
+  if ($self->transaction_depth == 1) {
+    $self->debugobj->txn_rollback() if $self->debug;
+    $self->_exec_txn_rollback;
+    $self->{transaction_depth}--;
+  }
+  elsif ($self->transaction_depth > 1) {
+    $self->{transaction_depth}--;
+
+    if ($self->auto_savepoint) {
+      $self->svp_rollback;
+      $self->svp_release;
+    }
+    else {
+      DBIx::Class::Storage::NESTED_ROLLBACK_EXCEPTION->throw(
+        "A txn_rollback in nested transaction is ineffective! (depth $self->{transaction_depth})"
+      );
+    }
+  }
+  else {
+    $self->throw_exception( 'Refusing to roll back without a started transaction' );
+  }
+}
 
 =head2 svp_begin
 
@@ -272,7 +351,30 @@ is provided, a random name will be used.
 
 =cut
 
-sub svp_begin { die "Virtual method!" }
+sub svp_begin {
+  my ($self, $name) = @_;
+
+  $self->throw_exception ("You can't use savepoints outside a transaction")
+    unless $self->transaction_depth;
+
+  my $exec = $self->can('_exec_svp_begin')
+    or $self->throw_exception ("Your Storage implementation doesn't support savepoints");
+
+  $name = $self->_svp_generate_name
+    unless defined $name;
+
+  push @{ $self->{savepoints} }, $name;
+
+  $self->debugobj->svp_begin($name) if $self->debug;
+
+  $exec->($self, $name);
+}
+
+sub _svp_generate_name {
+  my ($self) = @_;
+  return 'savepoint_'.scalar(@{ $self->{'savepoints'} });
+}
+
 
 =head2 svp_release
 
@@ -284,7 +386,35 @@ release all savepoints created after the one explicitly released as well.
 
 =cut
 
-sub svp_release { die "Virtual method!" }
+sub svp_release {
+  my ($self, $name) = @_;
+
+  $self->throw_exception ("You can't use savepoints outside a transaction")
+    unless $self->transaction_depth;
+
+  my $exec = $self->can('_exec_svp_release')
+    or $self->throw_exception ("Your Storage implementation doesn't support savepoints");
+
+  if (defined $name) {
+    my @stack = @{ $self->savepoints };
+    my $svp;
+
+    do { $svp = pop @stack } until $svp eq $name;
+
+    $self->throw_exception ("Savepoint '$name' does not exist")
+      unless $svp;
+
+    $self->savepoints(\@stack); # put back what's left
+  }
+  else {
+    $name = pop @{ $self->savepoints }
+      or $self->throw_exception('No savepoints to release');;
+  }
+
+  $self->debugobj->svp_release($name) if $self->debug;
+
+  $exec->($self, $name);
+}
 
 =head2 svp_rollback
 
@@ -296,7 +426,39 @@ release all savepoints created after the savepoint we rollback to.
 
 =cut
 
-sub svp_rollback { die "Virtual method!" }
+sub svp_rollback {
+  my ($self, $name) = @_;
+
+  $self->throw_exception ("You can't use savepoints outside a transaction")
+    unless $self->transaction_depth;
+
+  my $exec = $self->can('_exec_svp_rollback')
+    or $self->throw_exception ("Your Storage implementation doesn't support savepoints");
+
+  if (defined $name) {
+    my @stack = @{ $self->savepoints };
+    my $svp;
+
+    # a rollback doesn't remove the named savepoint,
+    # only everything after it
+    while (@stack and $stack[-1] ne $name) {
+      pop @stack
+    };
+
+    $self->throw_exception ("Savepoint '$name' does not exist")
+      unless @stack;
+
+    $self->savepoints(\@stack); # put back what's left
+  }
+  else {
+    $name = $self->savepoints->[-1]
+      or $self->throw_exception('No savepoints to rollback');;
+  }
+
+  $self->debugobj->svp_rollback($name) if $self->debug;
+
+  $exec->($self, $name);
+}
 
 =for comment
 
diff --git a/lib/DBIx/Class/Storage/DBI.pm b/lib/DBIx/Class/Storage/DBI.pm
index 04c33c1..f14eb97 100644
--- a/lib/DBIx/Class/Storage/DBI.pm
+++ b/lib/DBIx/Class/Storage/DBI.pm
@@ -32,8 +32,7 @@ __PACKAGE__->sql_name_sep('.');
 
 __PACKAGE__->mk_group_accessors('simple' => qw/
   _connect_info _dbi_connect_info _dbic_connect_attributes _driver_determined
-  _dbh _dbh_details _conn_pid _sql_maker _sql_maker_opts
-  transaction_depth _dbh_autocommit  savepoints
+  _dbh _dbh_details _conn_pid _sql_maker _sql_maker_opts _dbh_autocommit
 /);
 
 # the values for these accessors are picked out (and deleted) from
@@ -86,6 +85,7 @@ my @rdbms_specific_methods = qw/
   build_datetime_parser
   datetime_parser_type
 
+  txn_begin
   insert
   insert_bulk
   update
@@ -131,7 +131,6 @@ for my $meth (@rdbms_specific_methods) {
   };
 }
 
-
 =head1 NAME
 
 DBIx::Class::Storage::DBI - DBI storage handler
@@ -167,11 +166,9 @@ documents DBI-specific methods and behaviors.
 sub new {
   my $new = shift->next::method(@_);
 
-  $new->transaction_depth(0);
   $new->_sql_maker_opts({});
   $new->_dbh_details({});
-  $new->{savepoints} = [];
-  $new->{_in_dbh_do} = 0;
+  $new->{_in_do_block} = 0;
   $new->{_dbh_gen} = 0;
 
   # read below to see what this does
@@ -216,6 +213,9 @@ sub new {
       next unless $_;
       $_->{_dbh_gen}++;  # so that existing cursors will drop as well
       $_->_dbh(undef);
+
+      $_->transaction_depth(0);
+      $_->savepoints([]);
     }
   }
 }
@@ -243,6 +243,8 @@ sub _verify_pid {
     $dbh->{InactiveDestroy} = 1;
     $self->{_dbh_gen}++;
     $self->_dbh(undef);
+    $self->transaction_depth(0);
+    $self->savepoints([]);
   }
 
   return;
@@ -775,98 +777,33 @@ sub dbh_do {
   my $dbh = $self->_get_dbh;
 
   return $self->$code($dbh, @_)
-    if ( $self->{_in_dbh_do} || $self->{transaction_depth} );
+    if ( $self->{_in_do_block} || $self->{transaction_depth} );
 
-  local $self->{_in_dbh_do} = 1;
+  local $self->{_in_do_block} = 1;
 
   # take a ref instead of a copy, to preserve coderef @_ aliasing semantics
   my $args = \@_;
-  return try {
+
+  try {
     $self->$code ($dbh, @$args);
   } catch {
     $self->throw_exception($_) if $self->connected;
 
     # We were not connected - reconnect and retry, but let any
     #  exception fall right through this time
-    carp "Retrying $code after catching disconnected exception: $_"
-      if $ENV{DBIC_DBIRETRY_DEBUG};
+    carp "Retrying dbh_do($code) after catching disconnected exception: $_"
+      if $ENV{DBIC_STORAGE_RETRY_DEBUG};
 
     $self->_populate_dbh;
     $self->$code($self->_dbh, @$args);
   };
 }
 
-# This is basically a blend of dbh_do above and DBIx::Class::Storage::txn_do.
-# It also informs dbh_do to bypass itself while under the direction of txn_do,
-# via $self->{_in_dbh_do} (this saves some redundant eval and errorcheck, etc)
 sub txn_do {
-  my $self = shift;
-  my $coderef = shift;
-
-  ref $coderef eq 'CODE' or $self->throw_exception
-    ('$coderef must be a CODE reference');
-
-  local $self->{_in_dbh_do} = 1;
-
-  my @result;
-  my $want = wantarray;
-
-  my $tried = 0;
-  while(1) {
-    my $exception;
-
-    # take a ref instead of a copy, to preserve coderef @_ aliasing semantics
-    my $args = \@_;
-
-    try {
-      $self->txn_begin;
-      my $txn_start_depth = $self->transaction_depth;
-      if($want) {
-          @result = $coderef->(@$args);
-      }
-      elsif(defined $want) {
-          $result[0] = $coderef->(@$args);
-      }
-      else {
-          $coderef->(@$args);
-      }
-
-      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 wantarray ? @result : $result[0] }
-
-    if($self->transaction_depth > 1 || $tried++ || $self->connected) {
-      my $rollback_exception;
-      try { $self->txn_rollback } catch { $rollback_exception = shift };
-      if(defined $rollback_exception) {
-        my $exception_class = "DBIx::Class::Storage::NESTED_ROLLBACK_EXCEPTION";
-        $self->throw_exception($exception)  # propagate nested rollback
-          if $rollback_exception =~ /$exception_class/;
-
-        $self->throw_exception(
-          "Transaction aborted: ${exception}. "
-          . "Rollback failed: ${rollback_exception}"
-        );
-      }
-      $self->throw_exception($exception)
-    }
-
-    # We were not connected, and was first try - reconnect and retry
-    # via the while loop
-    carp "Retrying $coderef after catching disconnected exception: $exception"
-      if $ENV{DBIC_TXNRETRY_DEBUG};
-    $self->_populate_dbh;
-  }
+  # connects or reconnects on pid change, necessary to grab correct txn_depth
+  $_[0]->_get_dbh;
+  local $_[0]->{_in_do_block} = 1;
+  shift->next::method(@_);
 }
 
 =head2 disconnect
@@ -887,7 +824,8 @@ sub disconnect {
 
     $self->_do_connection_actions(disconnect_call_ => $_) for @actions;
 
-    $self->_dbh_rollback unless $self->_dbh_autocommit;
+    # stops the "implicit rollback on disconnect" warning
+    $self->_exec_txn_rollback unless $self->_dbh_autocommit;
 
     %{ $self->_dbh->{CachedKids} } = ();
     $self->_dbh->disconnect;
@@ -1370,118 +1308,23 @@ sub _connect {
   $dbh;
 }
 
-sub svp_begin {
-  my ($self, $name) = @_;
-
-  $name = $self->_svp_generate_name
-    unless defined $name;
-
-  $self->throw_exception ("You can't use savepoints outside a transaction")
-    if $self->{transaction_depth} == 0;
-
-  $self->throw_exception ("Your Storage implementation doesn't support savepoints")
-    unless $self->can('_svp_begin');
-
-  push @{ $self->{savepoints} }, $name;
-
-  $self->debugobj->svp_begin($name) if $self->debug;
-
-  return $self->_svp_begin($name);
-}
-
-sub svp_release {
-  my ($self, $name) = @_;
-
-  $self->throw_exception ("You can't use savepoints outside a transaction")
-    if $self->{transaction_depth} == 0;
-
-  $self->throw_exception ("Your Storage implementation doesn't support savepoints")
-    unless $self->can('_svp_release');
-
-  if (defined $name) {
-    $self->throw_exception ("Savepoint '$name' does not exist")
-      unless grep { $_ eq $name } @{ $self->{savepoints} };
-
-    # Dig through the stack until we find the one we are releasing.  This keeps
-    # the stack up to date.
-    my $svp;
-
-    do { $svp = pop @{ $self->{savepoints} } } while $svp ne $name;
-  } else {
-    $name = pop @{ $self->{savepoints} };
-  }
-
-  $self->debugobj->svp_release($name) if $self->debug;
-
-  return $self->_svp_release($name);
-}
-
-sub svp_rollback {
-  my ($self, $name) = @_;
-
-  $self->throw_exception ("You can't use savepoints outside a transaction")
-    if $self->{transaction_depth} == 0;
-
-  $self->throw_exception ("Your Storage implementation doesn't support savepoints")
-    unless $self->can('_svp_rollback');
-
-  if (defined $name) {
-      # If they passed us a name, verify that it exists in the stack
-      unless(grep({ $_ eq $name } @{ $self->{savepoints} })) {
-          $self->throw_exception("Savepoint '$name' does not exist!");
-      }
-
-      # Dig through the stack until we find the one we are releasing.  This keeps
-      # the stack up to date.
-      while(my $s = pop(@{ $self->{savepoints} })) {
-          last if($s eq $name);
-      }
-      # Add the savepoint back to the stack, as a rollback doesn't remove the
-      # named savepoint, only everything after it.
-      push(@{ $self->{savepoints} }, $name);
-  } else {
-      # We'll assume they want to rollback to the last savepoint
-      $name = $self->{savepoints}->[-1];
-  }
-
-  $self->debugobj->svp_rollback($name) if $self->debug;
-
-  return $self->_svp_rollback($name);
-}
-
-sub _svp_generate_name {
-  my ($self) = @_;
-  return 'savepoint_'.scalar(@{ $self->{'savepoints'} });
-}
-
 sub txn_begin {
   my $self = shift;
 
   # this means we have not yet connected and do not know the AC status
-  # (e.g. coderef $dbh)
+  # (e.g. coderef $dbh), need a full-fledged connection check
   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)
+  # Otherwise simply connect or re-connect on pid changes
   else {
     $self->_get_dbh;
   }
 
-  if($self->transaction_depth == 0) {
-    $self->debugobj->txn_begin()
-      if $self->debug;
-    $self->_dbh_begin_work;
-  }
-  elsif ($self->auto_savepoint) {
-    $self->svp_begin;
-  }
-  $self->{transaction_depth}++;
+  $self->next::method(@_);
 }
 
-sub _dbh_begin_work {
+sub _exec_txn_begin {
   my $self = shift;
 
   # if the user is utilizing txn_do - good for him, otherwise we need to
@@ -1489,7 +1332,7 @@ sub _dbh_begin_work {
   # We do this via ->dbh_do instead of ->dbh, so that the ->dbh "ping"
   # will be replaced by a failure of begin_work itself (which will be
   # then retried on reconnect)
-  if ($self->{_in_dbh_do}) {
+  if ($self->{_in_do_block}) {
     $self->_dbh->begin_work;
   } else {
     $self->dbh_do(sub { $_[1]->begin_work });
@@ -1498,83 +1341,76 @@ sub _dbh_begin_work {
 
 sub txn_commit {
   my $self = shift;
-  if (! $self->_dbh) {
-    $self->throw_exception('cannot COMMIT on a disconnected handle');
-  }
-  elsif ($self->{transaction_depth} == 1) {
-    $self->debugobj->txn_commit()
-      if ($self->debug);
-    $self->_dbh_commit;
-    $self->{transaction_depth} = 0
-      if $self->_dbh_autocommit;
-  }
-  elsif($self->{transaction_depth} > 1) {
-    $self->{transaction_depth}--;
-    $self->svp_release
-      if $self->auto_savepoint;
-  }
-  elsif (! $self->_dbh->FETCH('AutoCommit') ) {
 
-    carp "Storage transaction_depth $self->{transaction_depth} does not match "
-        ."false AutoCommit of $self->{_dbh}, attempting COMMIT anyway";
+  $self->_verify_pid if $self->_dbh;
+  $self->throw_exception("Unable to txn_commit() on a disconnected storage")
+    unless $self->_dbh;
 
-    $self->debugobj->txn_commit()
-      if ($self->debug);
-    $self->_dbh_commit;
-    $self->{transaction_depth} = 0
-      if $self->_dbh_autocommit;
-  }
-  else {
-    $self->throw_exception( 'Refusing to commit without a started transaction' );
+  # esoteric case for folks using external $dbh handles
+  if (! $self->transaction_depth and ! $self->_dbh->FETCH('AutoCommit') ) {
+    carp "Storage transaction_depth 0 does not match "
+        ."false AutoCommit of $self->{_dbh}, attempting COMMIT anyway";
+    $self->transaction_depth(1);
   }
+
+  $self->next::method(@_);
+
+  # if AutoCommit is disabled txn_depth never goes to 0
+  # as a new txn is started immediately on commit
+  $self->transaction_depth(1) if (
+    !$self->transaction_depth
+      and 
+    defined $self->_dbh_autocommit
+      and
+    ! $self->_dbh_autocommit
+  );
 }
 
-sub _dbh_commit {
-  my $self = shift;
-  my $dbh  = $self->_dbh
-    or $self->throw_exception('cannot COMMIT on a disconnected handle');
-  $dbh->commit;
+sub _exec_txn_commit {
+  shift->_dbh->commit;
 }
 
 sub txn_rollback {
   my $self = shift;
-  my $dbh = $self->_dbh;
-  try {
-    if ($self->{transaction_depth} == 1) {
-      $self->debugobj->txn_rollback()
-        if ($self->debug);
-      $self->{transaction_depth} = 0
-        if $self->_dbh_autocommit;
-      $self->_dbh_rollback;
-    }
-    elsif($self->{transaction_depth} > 1) {
-      $self->{transaction_depth}--;
-      if ($self->auto_savepoint) {
-        $self->svp_rollback;
-        $self->svp_release;
-      }
-    }
-    else {
-      die DBIx::Class::Storage::NESTED_ROLLBACK_EXCEPTION->new;
-    }
+
+  $self->_verify_pid if $self->_dbh;
+  $self->throw_exception("Unable to txn_rollback() on a disconnected storage")
+    unless $self->_dbh;
+
+  # esoteric case for folks using external $dbh handles
+  if (! $self->transaction_depth and ! $self->_dbh->FETCH('AutoCommit') ) {
+    carp "Storage transaction_depth 0 does not match "
+        ."false AutoCommit of $self->{_dbh}, attempting ROLLBACK anyway";
+    $self->transaction_depth(1);
   }
-  catch {
-    my $exception_class = "DBIx::Class::Storage::NESTED_ROLLBACK_EXCEPTION";
 
-    if ($_ !~ /$exception_class/) {
-      # ensure that a failed rollback resets the transaction depth
-      $self->{transaction_depth} = $self->_dbh_autocommit ? 0 : 1;
-    }
+  $self->next::method(@_);
 
-    $self->throw_exception($_)
-  };
+  # if AutoCommit is disabled txn_depth never goes to 0
+  # as a new txn is started immediately on commit
+  $self->transaction_depth(1) if (
+    !$self->transaction_depth
+      and 
+    defined $self->_dbh_autocommit
+      and
+    ! $self->_dbh_autocommit
+  );
 }
 
-sub _dbh_rollback {
-  my $self = shift;
-  my $dbh  = $self->_dbh
-    or $self->throw_exception('cannot ROLLBACK on a disconnected handle');
-  $dbh->rollback;
+sub _exec_txn_rollback {
+  shift->_dbh->rollback;
+}
+
+# generate some identical methods
+for my $meth (qw/svp_begin svp_release svp_rollback/) {
+  no strict qw/refs/;
+  *{__PACKAGE__ ."::$meth"} = subname $meth => sub {
+    my $self = shift;
+    $self->_verify_pid if $self->_dbh;
+    $self->throw_exception("Unable to $meth() on a disconnected storage")
+      unless $self->_dbh;
+    $self->next::method(@_);
+  };
 }
 
 # This used to be the top-half of _execute.  It was split out to make it
@@ -3091,7 +2927,8 @@ sub _is_text_lob_type {
 
 DBIx::Class can do some wonderful magic with handling exceptions,
 disconnections, and transactions when you use C<< AutoCommit => 1 >>
-(the default) combined with C<txn_do> for transaction support.
+(the default) combined with L<txn_do|DBIx::Class::Storage/txn_do> for
+transaction support.
 
 If you set C<< AutoCommit => 0 >> in your connect info, then you are always
 in an assumed transaction between commits, and you're telling us you'd
diff --git a/lib/DBIx/Class/Storage/DBI/ACCESS.pm b/lib/DBIx/Class/Storage/DBI/ACCESS.pm
index 723b856..35b076e 100644
--- a/lib/DBIx/Class/Storage/DBI/ACCESS.pm
+++ b/lib/DBIx/Class/Storage/DBI/ACCESS.pm
@@ -104,31 +104,23 @@ sub bind_attribute_by_data_type {
 # Unfortunately DBI does not support nested transactions.
 # WARNING: this code uses the undocumented 'BegunWork' DBI attribute.
 
-sub _svp_begin {
+sub _exec_svp_begin {
   my ($self, $name) = @_;
 
-  $self->throw_exception(
-    'cannot BEGIN a nested transaction on a disconnected handle'
-  ) unless $self->_dbh;
-
   local $self->_dbh->{AutoCommit} = 1;
   local $self->_dbh->{BegunWork}  = 0;
-  $self->_dbh_begin_work;
+  $self->_exec_txn_begin;
 }
 
 # A new nested transaction on the same level releases the previous one.
-sub _svp_release { 1 }
+sub _exec_svp_release { 1 }
 
-sub _svp_rollback {
+sub _exec_svp_rollback {
   my ($self, $name) = @_;
 
-  $self->throw_exception(
-    'cannot ROLLBACK a nested transaction on a disconnected handle'
-  ) unless $self->_dbh;
-
   local $self->_dbh->{AutoCommit} = 0;
   local $self->_dbh->{BegunWork}  = 1;
-  $self->_dbh_rollback;
+  $self->_exec_txn_rollback;
 }
 
 1;
diff --git a/lib/DBIx/Class/Storage/DBI/ADO/MS_Jet.pm b/lib/DBIx/Class/Storage/DBI/ADO/MS_Jet.pm
index 8475313..438db4e 100644
--- a/lib/DBIx/Class/Storage/DBI/ADO/MS_Jet.pm
+++ b/lib/DBIx/Class/Storage/DBI/ADO/MS_Jet.pm
@@ -78,14 +78,14 @@ sub _run_connection_actions {
 # (probably because of my nested transaction hacks in ACCESS.pm) fix it up
 # here.
 
-sub _dbh_commit {
+sub _exec_txn_commit {
   my $self = shift;
   $self->next::method(@_);
   $self->_dbh->{AutoCommit} = $self->_dbh_autocommit
     if $self->{transaction_depth} == 1;
 }
 
-sub _dbh_rollback {
+sub _exec_txn_rollback {
   my $self = shift;
   $self->next::method(@_);
   $self->_dbh->{AutoCommit} = $self->_dbh_autocommit
diff --git a/lib/DBIx/Class/Storage/DBI/Firebird/Common.pm b/lib/DBIx/Class/Storage/DBI/Firebird/Common.pm
index 3253b49..8b7e2a3 100644
--- a/lib/DBIx/Class/Storage/DBI/Firebird/Common.pm
+++ b/lib/DBIx/Class/Storage/DBI/Firebird/Common.pm
@@ -80,19 +80,19 @@ EOF
   return undef;
 }
 
-sub _svp_begin {
+sub _exec_svp_begin {
   my ($self, $name) = @_;
 
   $self->_dbh->do("SAVEPOINT $name");
 }
 
-sub _svp_release {
+sub _exec_svp_release {
   my ($self, $name) = @_;
 
   $self->_dbh->do("RELEASE SAVEPOINT $name");
 }
 
-sub _svp_rollback {
+sub _exec_svp_rollback {
   my ($self, $name) = @_;
 
   $self->_dbh->do("ROLLBACK TO SAVEPOINT $name")
diff --git a/lib/DBIx/Class/Storage/DBI/Informix.pm b/lib/DBIx/Class/Storage/DBI/Informix.pm
index 21401f4..cefd7c0 100644
--- a/lib/DBIx/Class/Storage/DBI/Informix.pm
+++ b/lib/DBIx/Class/Storage/DBI/Informix.pm
@@ -44,19 +44,19 @@ sub last_insert_id {
   shift->__last_insert_id;
 }
 
-sub _svp_begin {
+sub _exec_svp_begin {
     my ($self, $name) = @_;
 
-    $self->_get_dbh->do("SAVEPOINT $name");
+    $self->_dbh->do("SAVEPOINT $name");
 }
 
 # can't release savepoints
-sub _svp_release { 1 }
+sub _exec_svp_release { 1 }
 
-sub _svp_rollback {
+sub _exec_svp_rollback {
     my ($self, $name) = @_;
 
-    $self->_get_dbh->do("ROLLBACK TO SAVEPOINT $name")
+    $self->_dbh->do("ROLLBACK TO SAVEPOINT $name")
 }
 
 sub with_deferred_fk_checks {
diff --git a/lib/DBIx/Class/Storage/DBI/MSSQL.pm b/lib/DBIx/Class/Storage/DBI/MSSQL.pm
index 46f5828..e0ff02f 100644
--- a/lib/DBIx/Class/Storage/DBI/MSSQL.pm
+++ b/lib/DBIx/Class/Storage/DBI/MSSQL.pm
@@ -170,19 +170,19 @@ sub _select_args_to_query {
 
 # savepoint syntax is the same as in Sybase ASE
 
-sub _svp_begin {
+sub _exec_svp_begin {
   my ($self, $name) = @_;
 
-  $self->_get_dbh->do("SAVE TRANSACTION $name");
+  $self->_dbh->do("SAVE TRANSACTION $name");
 }
 
 # A new SAVE TRANSACTION with the same name releases the previous one.
-sub _svp_release { 1 }
+sub _exec_svp_release { 1 }
 
-sub _svp_rollback {
+sub _exec_svp_rollback {
   my ($self, $name) = @_;
 
-  $self->_get_dbh->do("ROLLBACK TRANSACTION $name");
+  $self->_dbh->do("ROLLBACK TRANSACTION $name");
 }
 
 sub sqlt_type { 'SQLServer' }
diff --git a/lib/DBIx/Class/Storage/DBI/ODBC/Firebird.pm b/lib/DBIx/Class/Storage/DBI/ODBC/Firebird.pm
index 940b944..133dcc1 100644
--- a/lib/DBIx/Class/Storage/DBI/ODBC/Firebird.pm
+++ b/lib/DBIx/Class/Storage/DBI/ODBC/Firebird.pm
@@ -31,9 +31,9 @@ makes it more suitable for long running processes such as under L<Catalyst>.
 __PACKAGE__->datetime_parser_type ('DBIx::Class::Storage::DBI::ODBC::Firebird::DateTime::Format');
 
 # releasing savepoints doesn't work for some reason, but that shouldn't matter
-sub _svp_release { 1 }
+sub _exec_svp_release { 1 }
 
-sub _svp_rollback {
+sub _exec_svp_rollback {
   my ($self, $name) = @_;
 
   try {
diff --git a/lib/DBIx/Class/Storage/DBI/Oracle/Generic.pm b/lib/DBIx/Class/Storage/DBI/Oracle/Generic.pm
index 50867e5..b9bf095 100644
--- a/lib/DBIx/Class/Storage/DBI/Oracle/Generic.pm
+++ b/lib/DBIx/Class/Storage/DBI/Oracle/Generic.pm
@@ -552,18 +552,18 @@ sub _prep_for_execute {
 
 # Savepoints stuff.
 
-sub _svp_begin {
+sub _exec_svp_begin {
   my ($self, $name) = @_;
-  $self->_get_dbh->do("SAVEPOINT $name");
+  $self->_dbh->do("SAVEPOINT $name");
 }
 
 # Oracle automatically releases a savepoint when you start another one with the
 # same name.
-sub _svp_release { 1 }
+sub _exec_svp_release { 1 }
 
-sub _svp_rollback {
+sub _exec_svp_rollback {
   my ($self, $name) = @_;
-  $self->_get_dbh->do("ROLLBACK TO SAVEPOINT $name")
+  $self->_dbh->do("ROLLBACK TO SAVEPOINT $name")
 }
 
 =head2 relname_to_table_alias
diff --git a/lib/DBIx/Class/Storage/DBI/Pg.pm b/lib/DBIx/Class/Storage/DBI/Pg.pm
index adf089d..f4dbda6 100644
--- a/lib/DBIx/Class/Storage/DBI/Pg.pm
+++ b/lib/DBIx/Class/Storage/DBI/Pg.pm
@@ -184,22 +184,22 @@ sub bind_attribute_by_data_type {
   $type_cache->{$data_type};
 }
 
-sub _svp_begin {
+sub _exec_svp_begin {
     my ($self, $name) = @_;
 
-    $self->_get_dbh->pg_savepoint($name);
+    $self->_dbh->pg_savepoint($name);
 }
 
-sub _svp_release {
+sub _exec_svp_release {
     my ($self, $name) = @_;
 
-    $self->_get_dbh->pg_release($name);
+    $self->_dbh->pg_release($name);
 }
 
-sub _svp_rollback {
+sub _exec_svp_rollback {
     my ($self, $name) = @_;
 
-    $self->_get_dbh->pg_rollback_to($name);
+    $self->_dbh->pg_rollback_to($name);
 }
 
 sub deployment_statements {
diff --git a/lib/DBIx/Class/Storage/DBI/Replicated.pm b/lib/DBIx/Class/Storage/DBI/Replicated.pm
index 9a9e05f..33e553d 100644
--- a/lib/DBIx/Class/Storage/DBI/Replicated.pm
+++ b/lib/DBIx/Class/Storage/DBI/Replicated.pm
@@ -268,6 +268,9 @@ my $method_dispatch = {
     txn_commit
     txn_rollback
     txn_scope_guard
+    _exec_txn_rollback
+    _exec_txn_begin
+    _exec_txn_commit
     deploy
     with_deferred_fk_checks
     dbh_do
@@ -295,14 +298,12 @@ my $method_dispatch = {
     _dbh_execute_array
     _sql_maker
     _per_row_update_delete
-    _dbh_begin_work
     _dbh_execute_inserts_with_no_binds
     _select_args_to_query
     _svp_generate_name
     _multipk_update_delete
     _normalize_connect_info
     _parse_connect_do
-    _dbh_commit
     _execute_array
     savepoints
     _sql_maker_opts
@@ -311,7 +312,6 @@ my $method_dispatch = {
     _native_data_type
     _get_dbh
     sql_maker_class
-    _dbh_rollback
     _adjust_select_args_for_complex_prefetch
     _resolve_ident_sources
     _resolve_column_info
diff --git a/lib/DBIx/Class/Storage/DBI/SQLAnywhere.pm b/lib/DBIx/Class/Storage/DBI/SQLAnywhere.pm
index 542dd56..24af0cf 100644
--- a/lib/DBIx/Class/Storage/DBI/SQLAnywhere.pm
+++ b/lib/DBIx/Class/Storage/DBI/SQLAnywhere.pm
@@ -182,19 +182,19 @@ sub connect_call_datetime_setup {
   );
 }
 
-sub _svp_begin {
+sub _exec_svp_begin {
     my ($self, $name) = @_;
 
-    $self->_get_dbh->do("SAVEPOINT $name");
+    $self->_dbh->do("SAVEPOINT $name");
 }
 
 # can't release savepoints that have been rolled back
-sub _svp_release { 1 }
+sub _exec_svp_release { 1 }
 
-sub _svp_rollback {
+sub _exec_svp_rollback {
     my ($self, $name) = @_;
 
-    $self->_get_dbh->do("ROLLBACK TO SAVEPOINT $name")
+    $self->_dbh->do("ROLLBACK TO SAVEPOINT $name")
 }
 
 1;
diff --git a/lib/DBIx/Class/Storage/DBI/Sybase/ASE.pm b/lib/DBIx/Class/Storage/DBI/Sybase/ASE.pm
index f70db66..a7cf298 100644
--- a/lib/DBIx/Class/Storage/DBI/Sybase/ASE.pm
+++ b/lib/DBIx/Class/Storage/DBI/Sybase/ASE.pm
@@ -878,7 +878,7 @@ sub connect_call_datetime_setup {
 }
 
 
-sub _dbh_begin_work {
+sub _exec_txn_begin {
   my $self = shift;
 
 # bulkLogin=1 connections are always in a transaction, and can only call BEGIN
@@ -892,19 +892,19 @@ sub _dbh_begin_work {
 
 # savepoint support using ASE syntax
 
-sub _svp_begin {
+sub _exec_svp_begin {
   my ($self, $name) = @_;
 
-  $self->_get_dbh->do("SAVE TRANSACTION $name");
+  $self->_dbh->do("SAVE TRANSACTION $name");
 }
 
 # A new SAVE TRANSACTION with the same name releases the previous one.
-sub _svp_release { 1 }
+sub _exec_svp_release { 1 }
 
-sub _svp_rollback {
+sub _exec_svp_rollback {
   my ($self, $name) = @_;
 
-  $self->_get_dbh->do("ROLLBACK TRANSACTION $name");
+  $self->_dbh->do("ROLLBACK TRANSACTION $name");
 }
 
 1;
diff --git a/lib/DBIx/Class/Storage/DBI/Sybase/FreeTDS.pm b/lib/DBIx/Class/Storage/DBI/Sybase/FreeTDS.pm
index feb50fe..9f5ba5d 100644
--- a/lib/DBIx/Class/Storage/DBI/Sybase/FreeTDS.pm
+++ b/lib/DBIx/Class/Storage/DBI/Sybase/FreeTDS.pm
@@ -74,10 +74,10 @@ sub set_textsize {
   $self->_dbh->do("SET TEXTSIZE $text_size");
 }
 
-sub _dbh_begin_work {
+sub _exec_txn_begin {
   my $self = shift;
 
-  if ($self->{_in_dbh_do}) {
+  if ($self->{_in_do_block}) {
     $self->_dbh->do('BEGIN TRAN');
   }
   else {
@@ -85,7 +85,7 @@ sub _dbh_begin_work {
   }
 }
 
-sub _dbh_commit {
+sub _exec_txn_commit {
   my $self = shift;
 
   my $dbh = $self->_dbh
@@ -94,7 +94,7 @@ sub _dbh_commit {
   $dbh->do('COMMIT');
 }
 
-sub _dbh_rollback {
+sub _exec_txn_rollback {
   my $self = shift;
 
   my $dbh  = $self->_dbh
diff --git a/lib/DBIx/Class/Storage/DBI/mysql.pm b/lib/DBIx/Class/Storage/DBI/mysql.pm
index fcf9fbf..962f6bd 100644
--- a/lib/DBIx/Class/Storage/DBI/mysql.pm
+++ b/lib/DBIx/Class/Storage/DBI/mysql.pm
@@ -88,22 +88,22 @@ sub deployment_statements {
   $self->next::method($schema, $type, $version, $dir, $sqltargs, @rest);
 }
 
-sub _svp_begin {
+sub _exec_svp_begin {
     my ($self, $name) = @_;
 
-    $self->_get_dbh->do("SAVEPOINT $name");
+    $self->_dbh->do("SAVEPOINT $name");
 }
 
-sub _svp_release {
+sub _exec_svp_release {
     my ($self, $name) = @_;
 
-    $self->_get_dbh->do("RELEASE SAVEPOINT $name");
+    $self->_dbh->do("RELEASE SAVEPOINT $name");
 }
 
-sub _svp_rollback {
+sub _exec_svp_rollback {
     my ($self, $name) = @_;
 
-    $self->_get_dbh->do("ROLLBACK TO SAVEPOINT $name")
+    $self->_dbh->do("ROLLBACK TO SAVEPOINT $name")
 }
 
 sub is_replicating {
diff --git a/lib/DBIx/Class/Storage/TxnScopeGuard.pm b/lib/DBIx/Class/Storage/TxnScopeGuard.pm
index d5291fa..5b72f68 100644
--- a/lib/DBIx/Class/Storage/TxnScopeGuard.pm
+++ b/lib/DBIx/Class/Storage/TxnScopeGuard.pm
@@ -122,7 +122,11 @@ sub DESTROY {
     try { $storage->_seems_connected && $storage->txn_rollback }
     catch { $rollback_exception = shift };
 
-    if (defined $rollback_exception && $rollback_exception !~ /DBIx::Class::Storage::NESTED_ROLLBACK_EXCEPTION/) {
+    if ( $rollback_exception and (
+      ! defined blessed $rollback_exception
+          or
+      ! $rollback_exception->isa('DBIx::Class::Storage::NESTED_ROLLBACK_EXCEPTION')
+    ) ) {
       # append our text - THIS IS A TEMPORARY FIXUP!
       # a real stackable exception object is in the works
       if (ref $exception eq 'DBIx::Class::Exception') {
diff --git a/t/storage/txn.t b/t/storage/txn.t
index f84fa9a..4d46e5b 100644
--- a/t/storage/txn.t
+++ b/t/storage/txn.t
@@ -160,13 +160,13 @@ for my $want (0,1) {
         my $guard = $schema->txn_scope_guard;
         $schema->txn_do( sub { die } );
       };
+      is( $schema->storage->transaction_depth, 0, 'Transaction successfully aborted' );
       $schema->txn_do( sub {
         ok ($schema->storage->_dbh->do ('SELECT 1'), "Query after exceptions ok ($_)");
       });
     }
 
     $schema->txn_do ( sub { _test_forking_action ($schema, $pass) } );
-
   }
 }
 
@@ -182,6 +182,7 @@ for my $want (0,1) {
         my $guard = $schema->txn_scope_guard;
         $schema->txn_do( sub { die } );
       };
+      is( $schema->storage->transaction_depth, 0, 'Transaction successfully aborted' );
       $schema->txn_do( sub {
         ok ($schema->storage->_dbh->do ('SELECT 1'), "Query after exceptions ok ($_)");
       });
@@ -283,14 +284,14 @@ my $fail_code = sub {
     my $artist = $schema->resultset('Artist')->find(3);
 
     # Force txn_rollback() to throw an exception
-    no warnings 'redefine';
-    no strict 'refs';
+    no warnings qw/once redefine/;
+
+    # this should logically work just fine - but it does not,
+    # only direct override of the existing method dtrt
+    #local *DBIx::Class::Storage::DBI::SQLite::txn_rollback = sub { die 'FAILED' };
 
-    # die in rollback
-    local *{"DBIx::Class::Storage::DBI::SQLite::txn_rollback"} = sub{
-      my $storage = shift;
-      die 'FAILED';
-    };
+    local *DBIx::Class::Storage::DBI::txn_rollback = sub { die 'FAILED' };
+    Class::C3->reinitialize() if DBIx::Class::_ENV_::OLD_MRO;
 
     throws_ok (
       sub {
@@ -348,7 +349,9 @@ my $fail_code = sub {
   ok(!defined($cd), q{failed txn_do didn't add failed txn's cd});
 }
 
+
 # Grab a new schema to test txn before connect
+# also test nested txn exception
 {
   my $schema = DBICTest->init_schema(no_deploy => 1);
   lives_ok (sub {
@@ -356,9 +359,7 @@ my $fail_code = sub {
     $schema->txn_begin();
   }, 'Pre-connection nested transactions.');
 
-  # although not connected DBI would still warn about rolling back at disconnect
-  $schema->txn_rollback;
-  $schema->txn_rollback;
+  throws_ok( sub { $schema->txn_rollback }, 'DBIx::Class::Storage::NESTED_ROLLBACK_EXCEPTION', 'got proper nested rollback exception' );
 }
 
 # make sure AutoCommit => 0 on external handles behaves correctly with scope_guard