diff --git a/lib/DBIx/Class/Storage.pm b/lib/DBIx/Class/Storage.pm index dfff9a1a4..57eb77d2c 100644 --- a/lib/DBIx/Class/Storage.pm +++ b/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). 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 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 exception +to help you unwind to the outer-most transaction's scope +(assuming you are using L or L). +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 or +C. =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; diff --git a/lib/DBIx/Class/Storage/BlockRunner.pm b/lib/DBIx/Class/Storage/BlockRunner.pm index 64d5164cd..3140080fc 100644 --- a/lib/DBIx/Class/Storage/BlockRunner.pm +++ b/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; diff --git a/lib/DBIx/Class/Storage/DBI.pm b/lib/DBIx/Class/Storage/DBI.pm index 16d68e52c..82e146aaa 100644 --- a/lib/DBIx/Class/Storage/DBI.pm +++ b/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}; diff --git a/lib/DBIx/Class/Storage/DBI/Replicated.pm b/lib/DBIx/Class/Storage/DBI/Replicated.pm index 48642ece6..65432ddf3 100644 --- a/lib/DBIx/Class/Storage/DBI/Replicated.pm +++ b/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 diff --git a/t/storage/txn.t b/t/storage/txn.t index 0edca6c44..e7d4fd123 100644 --- a/t/storage/txn.t +++ b/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;