Deferred rollback

lib/DBIx/Class/Storage.pm

@@ -19,7 +19,7 @@ use DBIx::Class::Storage::TxnScopeGuard;
 use DBIx::Class::_Util qw( dbic_internal_try dbic_internal_catch fail_on_internal_call );
 use namespace::clean;
 
-__PACKAGE__->mk_group_accessors(simple => qw/debug schema transaction_depth auto_savepoint savepoints/);
+__PACKAGE__->mk_group_accessors(simple => qw/debug schema transaction_depth deferred_rollback auto_savepoint savepoints/);
 __PACKAGE__->mk_group_accessors(component_class => 'cursor_class');
 
 __PACKAGE__->cursor_class('DBIx::Class::Cursor');
@@ -177,6 +177,7 @@ transaction failure.
 
 sub txn_do {
   my $self = shift;
+  $self->_throw_deferred_rollback if $self->deferred_rollback;
 
   DBIx::Class::Storage::BlockRunner->new(
     storage => $self,
@@ -200,6 +201,7 @@ an entire code block to be executed transactionally.
 
 sub txn_begin {
   my $self = shift;
+  $self->_throw_deferred_rollback if $self->deferred_rollback;
 
   if($self->transaction_depth == 0) {
     $self->debugobj->txn_begin()
@@ -224,6 +226,7 @@ transaction currently in effect (i.e. you called L</txn_begin>).
 
 sub txn_commit {
   my $self = shift;
+  $self->_throw_deferred_rollback if $self->deferred_rollback;
 
   if ($self->transaction_depth == 1) {
     $self->debugobj->txn_commit() if $self->debug;
@@ -242,9 +245,18 @@ sub txn_commit {
 
 =head2 txn_rollback
 
-Issues a rollback of the current transaction. A nested rollback will
-throw a L<DBIx::Class::Storage::NESTED_ROLLBACK_EXCEPTION> exception,
-which allows the rollback to propagate to the outermost transaction.
+Issues a rollback of the current transaction (or savepoint, if
+auto_savepoint is enabled, and you are in a nested transaction).
+
+If you are in a nested transaction without auto_savepoint, rollback will
+put the storage into a "deferred rollback" state and throw a
+L<DBIx::Class::Storage::NESTED_ROLLBACK_EXCEPTION> exception
+to help you unwind to the outer-most transaction's scope
+(assuming you are using L</txn_do> or L</txn_scope_guard>).
+Until the "deferred rollback" condition is resolved,
+the storage engine will throw exceptions on any attempt to begin, commit,
+or rollback a transaction other than by exiting a C<txn_do> or
+C<txn_scope_guard>.
 
 =cut
 
@@ -253,6 +265,7 @@ sub txn_rollback {
 
   if ($self->transaction_depth == 1) {
     $self->debugobj->txn_rollback() if $self->debug;
+    $self->deferred_rollback(undef);
     $self->{transaction_depth}--;
 
     # in case things get really hairy - just disconnect
@@ -276,8 +289,10 @@ sub txn_rollback {
       $self->svp_release;
     }
     else {
+      $self->deferred_rollback(1);
       DBIx::Class::Storage::NESTED_ROLLBACK_EXCEPTION->throw(
         "A txn_rollback in nested transaction is ineffective! (depth $self->{transaction_depth})"
+        ." You must exit all transaction nesting levels before the rollback takes effect."
       );
     }
   }
@@ -286,6 +301,13 @@ sub txn_rollback {
   }
 }
 
+sub _throw_deferred_rollback {
+  DBIx::Class::Storage::NESTED_ROLLBACK_EXCEPTION->throw(
+    "You are in the middle of a deferred rollback from a nested transaction."
+    ." No further statements can be executed until the rollback is complete."
+  );
+}
+
 # to be called by several internal stacked transaction handler codepaths
 # not for external consumption
 # *DOES NOT* throw exceptions, instead:
@@ -294,21 +316,9 @@ sub txn_rollback {
 sub __delicate_rollback {
   my $self = shift;
 
-  if (
-    ( $self->transaction_depth || 0 ) > 1
-      and
-    # FIXME - the autosvp check here shouldn't be happening, it should be a role-ish thing
-    # The entire concept needs to be rethought with the storage layer... or something
-    ! $self->auto_savepoint
-      and
-    # the handle seems healthy, and there is nothing for us to do with it
-    # just go ahead and bow out, without triggering the txn_rollback() "nested exception"
-    # the unwind will eventually fail somewhere higher up if at all
-    # FIXME: a ::Storage::DBI-specific method, not a generic ::Storage one
-    $self->_seems_connected
-  ) {
-    # all above checks out - there is nothing to do on the $dbh itself
-    # just a plain soft-decrease of depth
+  # If nested scope requested a rollback and it can only be performed on the
+  # top-level transaction's scope, then just silently decrease the depth.
+  if ($self->deferred_rollback and ( $self->transaction_depth || 0 ) > 1) {
     $self->{transaction_depth}--;
     return;
   }
@@ -396,6 +406,9 @@ sub svp_begin {
   my $exec = $self->can('_exec_svp_begin')
     or $self->throw_exception ("Your Storage implementation doesn't support savepoints");
 
+  # This could happen if savepoints were not enabled at the time rollback was called
+  $self->_throw_deferred_rollback if $self->deferred_rollback;
+
   $name = $self->_svp_generate_name
     unless defined $name;
 
@@ -431,6 +444,9 @@ sub svp_release {
   my $exec = $self->can('_exec_svp_release')
     or $self->throw_exception ("Your Storage implementation doesn't support savepoints");
 
+  # This could happen if savepoints were not enabled at the time rollback was called
+  $self->_throw_deferred_rollback if $self->deferred_rollback;
+
   if (defined $name) {
     my @stack = @{ $self->savepoints };
     my $svp = '';
@@ -474,6 +490,9 @@ sub svp_rollback {
   my $exec = $self->can('_exec_svp_rollback')
     or $self->throw_exception ("Your Storage implementation doesn't support savepoints");
 
+  # This could happen if savepoints were not enabled at the time rollback was called
+  $self->_throw_deferred_rollback if $self->deferred_rollback;
+
   if (defined $name) {
     my @stack = @{ $self->savepoints };
     my $svp;

lib/DBIx/Class/Storage/BlockRunner.pm

@@ -92,6 +92,10 @@ sub run {
 
   my $storage = $self->storage;
 
+  # Don't get into the tangle of code below if we already know the storage is
+  # trying to rollback.
+  $storage->_throw_deferred_rollback if $storage->deferred_rollback;
+
   return $cref->( @_ ) if (
     $storage->{_in_do_block}
       and
@@ -153,6 +157,14 @@ sub _run {
           $cref,
         ) unless $delta_txn == 1 and $cur_depth == 0;
       }
+      elsif ($storage->deferred_rollback) {
+        # This means the inner code called 'rollback' in a case where savepoints
+        # weren't enabled, and then caught the exception.
+        carp 'A deferred rollback is in effect, but you exited a transaction-wrapped '
+           . 'block cleanly which normally implies "commit". '
+           . "You're getting a rollback instead.";
+        $storage->__delicate_rollback;
+      }
       else {
         dbic_internal_try {
           $storage->txn_commit;

lib/DBIx/Class/Storage/DBI.pm

@@ -909,6 +909,7 @@ sub disconnect {
     $self->_dbh(undef);
     $self->_dbh_details({});
     $self->transaction_depth(undef);
+    $self->deferred_rollback(undef);
     $self->_dbh_autocommit(undef);
     $self->savepoints([]);
 
@@ -1105,6 +1106,7 @@ sub _populate_dbh {
   # Always set the transaction depth on connect, since
   #  there is no transaction in progress by definition
   $_[0]->transaction_depth( $_[0]->_dbh_autocommit ? 0 : 1 );
+  $_[0]->deferred_rollback( undef );
 
   $_[0]->_run_connection_actions unless $_[0]->{_in_determine_driver};
 

lib/DBIx/Class/Storage/DBI/Replicated.pm

@@ -298,6 +298,8 @@ my $method_dispatch = {
     _dbi_attrs_for_bind
     bind_attribute_by_data_type
     transaction_depth
+    deferred_rollback
+    _throw_deferred_rollback
     _dbh
     _select_args
     _dbh_execute_for_fetch

t/storage/txn.t

@@ -379,6 +379,47 @@ my $fail_code = sub {
   throws_ok( sub { $schema->txn_rollback }, 'DBIx::Class::Storage::NESTED_ROLLBACK_EXCEPTION', 'got proper nested rollback exception' );
 }
 
+# Nested rollback should never result in a commit
+{
+  use Try::Tiny;
+  my $schema = DBICTest->init_schema();
+
+  is( $schema->storage->transaction_depth, 0, 'txn depth starts at 0');
+
+  my $artist = $schema->resultset('Artist')->find(3);
+
+  # This simulates a situation that might happen if a routine tries performing
+  # database work, but it fails, and then the code that called it wants
+  # to perform additional database work.  (like logging the exception to the DB)
+  my $code_with_error_handling= sub {
+    my $artist= shift;
+    try {
+      $schema->txn_do($fail_code, $artist);
+    } catch {
+      $code->($artist, 'catch code inserts records');
+    };
+  };
+
+  # ...and this simulates code that doesn't realize that wrapping the previous
+  # with a transaction causes it to break, because it can't perform a partial
+  # rollback of the first step.
+  # It should probably be an exception instead of a warning, but that could
+  # break existing code...
+  warnings_like( sub { $schema->txn_do($code_with_error_handling, $artist); },
+    qr/rollback/i, 'get warning about nested rollback' );
+
+  my $fail_record= $artist->cds({
+    title => 'this should not exist',
+    year => 2005,
+  })->first;
+  my $followup_record= $artist->cds({
+    title => 'catch code inserts records',
+    year => 2006,
+  })->first;
+  ok( !defined $fail_record, 'record from failcode not committed' );
+  ok( !defined $followup_record, 'record from exception handler not committed' );
+}
+
 # make sure AutoCommit => 0 on external handles behaves correctly with scope_guard
 warnings_are {
   my $factory = DBICTest->init_schema;