From: Peter Rabbitson Date: Tue, 26 Apr 2011 06:38:26 +0000 (+0200) Subject: Another overhaul of transaction/savepoint handling X-Git-Url: http://git.shadowcat.co.uk/gitweb/gitweb.cgi?a=commitdiff_plain;h=90d7422fc;p=dbsrgits%2FDBIx-Class-Historic.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). =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 for transaction support. +(the default) combined with L 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. __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