From edd2abd1f903c9d757b9ac83060dc7c53d6363b5 Mon Sep 17 00:00:00 2001 From: Michael Conrad Date: Tue, 24 Jan 2017 22:10:48 -0500 Subject: [PATCH 1/2] Test case for caught errors from nested rollback There are several ways that a nested rollback can cause un-intended behavior, but this is one I feel is pretty realistic. (In fact, it probably already happens in the wild somewhere.) In this case, if a subroutine wraps some code in a transaction, then catches errors from the code expecting the transaction to be rolled back, then performs some of its own database work without carefully inspecting the exception it caught, the whole thing can end up geting committed if it occurs within an inner transaction. --- t/storage/txn.t | 41 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) 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; From 777cae34e8717bcc6dca99be5c99eeaf184cbfb5 Mon Sep 17 00:00:00 2001 From: Michael Conrad Date: Tue, 24 Jan 2017 19:08:18 -0500 Subject: [PATCH 2/2] deferred_rollback: better handling of invalid nested rollbacks Currently, there is a messy case when savepoints aren't enabled and a user creates nested transactions and then calls 'rollback'. DBIC throws an exception in hopes that it propogates up the stack to the top-most transaction where it can be rolled back properly, but there is no guarantee that the user doesn't just catch the exception and then accidentally commit unwanted changes by exiting a txn_do block cleanly. This change is a proposed fix that sets a "deferred rollback" condition on the Storage object such that attempting any other transaction-related operation will fail until the top-level transaction has been rolled back. It also makes the BlockRunner aware of deferred rollbacks so that it performs a rollback instead of commit when a block exits cleanly. It also takes the somewhat bold step of stopping BlockRunner from starting new blocks at all until the condition is resolved... I stopped short of reaching into the individual Storage engines to stop them from executing further statements, though that is theoretically what we want to do. --- lib/DBIx/Class/Storage.pm | 57 ++++++++++++++++-------- lib/DBIx/Class/Storage/BlockRunner.pm | 12 +++++ lib/DBIx/Class/Storage/DBI.pm | 2 + lib/DBIx/Class/Storage/DBI/Replicated.pm | 2 + 4 files changed, 54 insertions(+), 19 deletions(-) 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