From: Peter Rabbitson Date: Thu, 21 Oct 2010 22:37:06 +0000 (+0200) Subject: Massive cleanup of transaction handlers X-Git-Url: http://git.shadowcat.co.uk/gitweb/gitweb.cgi?a=commitdiff_plain;h=7d216b101775c472b012b257d848f50d78b193c8;p=dbsrgits%2FDBIx-Class-Historic.git Massive cleanup of transaction handlers Audit the txn_do/scope_guard/txn_begin|txn_commit codepaths to make sure they can be interchangeably used and still just DTRT. Write a ton of tests to codify current behavior In the process revert d8a5a2340b0ac0dbf1c3e7779e7951b1bec2ccd5 which introduces a regression (now poperly caught by tests) --- diff --git a/lib/DBIx/Class/Storage/DBI.pm b/lib/DBIx/Class/Storage/DBI.pm index 6a85b2c..29fc1bf 100644 --- a/lib/DBIx/Class/Storage/DBI.pm +++ b/lib/DBIx/Class/Storage/DBI.pm @@ -778,8 +778,6 @@ sub txn_do { ref $coderef eq 'CODE' or $self->throw_exception ('$coderef must be a CODE reference'); - return $coderef->(@_) if $self->{transaction_depth} && ! $self->auto_savepoint; - local $self->{_in_dbh_do} = 1; my @result; @@ -794,6 +792,7 @@ sub txn_do { try { $self->txn_begin; + my $txn_start_depth = $self->transaction_depth; if($want_array) { @result = $coderef->(@$args); } @@ -803,14 +802,22 @@ sub txn_do { else { $coderef->(@$args); } - $self->txn_commit; + + 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 $want_array ? @result : $result[0] } - if($tried++ || $self->connected) { + if($self->transaction_depth > 1 || $tried++ || $self->connected) { my $rollback_exception; try { $self->txn_rollback } catch { $rollback_exception = shift }; if(defined $rollback_exception) { @@ -1368,9 +1375,8 @@ sub svp_rollback { } sub _svp_generate_name { - my ($self) = @_; - - return 'savepoint_'.scalar(@{ $self->{'savepoints'} }); + my ($self) = @_; + return 'savepoint_'.scalar(@{ $self->{'savepoints'} }); } sub txn_begin { @@ -1378,9 +1384,18 @@ sub txn_begin { # this means we have not yet connected and do not know the AC status # (e.g. coderef $dbh) - $self->ensure_connected if (! defined $self->_dbh_autocommit); + 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) + else { + $self->_get_dbh; + } - if($self->{transaction_depth} == 0) { + if($self->transaction_depth == 0) { $self->debugobj->txn_begin() if $self->debug; $self->_dbh_begin_work; @@ -1420,6 +1435,9 @@ sub txn_commit { $self->svp_release if $self->auto_savepoint; } + else { + $self->throw_exception( 'Refusing to commit without a started transaction' ); + } } sub _dbh_commit { diff --git a/lib/DBIx/Class/Storage/TxnScopeGuard.pm b/lib/DBIx/Class/Storage/TxnScopeGuard.pm index 7f93113..f686cce 100644 --- a/lib/DBIx/Class/Storage/TxnScopeGuard.pm +++ b/lib/DBIx/Class/Storage/TxnScopeGuard.pm @@ -4,13 +4,16 @@ use strict; use warnings; use Carp::Clan qw/^DBIx::Class/; use Try::Tiny; +use Scalar::Util qw/weaken/; use namespace::clean; sub new { my ($class, $storage) = @_; $storage->txn_begin; - bless [ 0, $storage ], ref $class || $class; + my $guard = bless [ 0, $storage, $storage->_dbh ], ref $class || $class; + weaken ($guard->[2]); + $guard; } sub commit { @@ -25,6 +28,10 @@ sub DESTROY { return if $dismiss; + # if our dbh is not ours anymore, the weakref will go undef + $storage->_preserve_foreign_dbh; + return unless $_[0]->[2]; + my $exception = $@; { @@ -34,7 +41,9 @@ sub DESTROY { unless $exception; my $rollback_exception; - try { $storage->txn_rollback } + # do minimal connectivity check due to weird shit like + # https://rt.cpan.org/Public/Bug/Display.html?id=62370 + try { $storage->_seems_connected && $storage->txn_rollback } catch { $rollback_exception = shift }; if (defined $rollback_exception && $rollback_exception !~ /DBIx::Class::Storage::NESTED_ROLLBACK_EXCEPTION/) { diff --git a/t/50fork.t b/t/50fork.t index df6957c..8fea72f 100644 --- a/t/50fork.t +++ b/t/50fork.t @@ -18,7 +18,7 @@ if($num_children !~ /^[0-9]+$/ || $num_children < 10) { $num_children = 10; } -plan tests => $num_children + 6; +plan tests => ($num_children*2) + 6; use lib qw(t/lib); @@ -76,18 +76,36 @@ while(@pids < $num_children) { $pid = $$; - my $child_rs = $schema->resultset('CD')->search({ year => 1901 }); - my $row = $parent_rs->next; - if($row && $row->get_column('artist') =~ /^(?:123|456)$/) { - $schema->resultset('CD')->create({ title => "test success $pid", artist => $pid, year => scalar(@pids) }); + my $work = sub { + my $child_rs = $schema->resultset('CD')->search({ year => 1901 }); + my $row = $parent_rs->next; + $schema->resultset('CD')->create({ title => "test success $pid", artist => $pid, year => scalar(@pids) }) + if($row && $row->get_column('artist') =~ /^(?:123|456)$/); + }; + + # try with and without transactions + if ((@pids % 3) == 1) { + my $guard = $schema->txn_scope_guard; + $work->(); + $guard->commit; } + elsif ((@pids % 3) == 2) { + $schema->txn_do ($work); + } + else { + $work->(); + } + sleep(3); - exit; + exit 0; } ok(1, "past forking"); -waitpid($_,0) for(@pids); +for (@pids) { + waitpid($_,0); + ok (! $?, "Child $_ exitted cleanly"); +}; ok(1, "past waiting"); diff --git a/t/storage/txn.t b/t/storage/txn.t index 87d1b45..89dddc5 100644 --- a/t/storage/txn.t +++ b/t/storage/txn.t @@ -99,6 +99,145 @@ my $code = sub { is( $schema->storage->{transaction_depth}, 0, 'txn depth has been reset'); } +# test nested txn_begin on fresh connection +{ + my $schema = DBICTest->init_schema(sqlite_use_file => 1, no_deploy => 1); + $schema->storage->ensure_connected; + + is ($schema->storage->transaction_depth, 0, 'Start outside txn'); + + my @pids; + for my $action ( + sub { + my $s = shift; + die "$$ starts in txn!" if $s->storage->transaction_depth != 0; + $s->txn_do ( sub { + die "$$ not in txn!" if $s->storage->transaction_depth == 0; + $s->storage->dbh->do('SELECT 1') } + ); + die "$$ did not finish txn!" if $s->storage->transaction_depth != 0; + }, + sub { + $_[0]->txn_begin; + $_[0]->storage->dbh->do('SELECT 1'); + $_[0]->txn_commit + }, + sub { + my $guard = $_[0]->txn_scope_guard; + $_[0]->storage->dbh->do('SELECT 1'); + $guard->commit + }, + ) { + push @pids, fork(); + die "Unable to fork: $!\n" + if ! defined $pids[-1]; + + if ($pids[-1]) { + next; + } + + $action->($schema); + exit 0; + } + + is ($schema->storage->transaction_depth, 0, 'Parent still outside txn'); + + for my $pid (@pids) { + waitpid ($pid, 0); + ok (! $?, "Child $pid exit ok"); + } +} + +# Test txn_do/scope_guard with forking: outer txn_do +{ + my $schema = DBICTest->init_schema( sqlite_use_file => 1 ); + + for my $pass (1..2) { + + # do something trying to destabilize the depth count + for (1..2) { + eval { + my $guard = $schema->txn_scope_guard; + $schema->txn_do( sub { die } ); + }; + $schema->txn_do( sub { + ok ($schema->storage->_dbh->do ('SELECT 1'), "Query after exceptions ok ($_)"); + }); + } + + for my $pid ( $schema->txn_do ( sub { _forking_action ($schema) } ) ) { + waitpid ($pid, 0); + ok (! $?, "Child $pid exit ok (pass $pass)"); + isa_ok ($schema->resultset ('Artist')->find ({ name => "forking action $pid" }), 'DBIx::Class::Row'); + } + } +} + +# same test with outer guard +{ + my $schema = DBICTest->init_schema( sqlite_use_file => 1 ); + + for my $pass (1..2) { + + # do something trying to destabilize the depth count + for (1..2) { + eval { + my $guard = $schema->txn_scope_guard; + $schema->txn_do( sub { die } ); + }; + $schema->txn_do( sub { + ok ($schema->storage->_dbh->do ('SELECT 1'), "Query after exceptions ok ($_)"); + }); + } + + my @pids; + my $guard = $schema->txn_scope_guard; + _forking_action ($schema); + $guard->commit; + + for my $pid (@pids) { + waitpid ($pid, 0); + ok (! $?, "Child $pid exit ok (pass $pass)"); + isa_ok ($schema->resultset ('Artist')->find ({ name => "forking action $pid" }), 'DBIx::Class::Row'); + } + } +} + +sub _forking_action { + my $schema = shift; + + my @pids; + while (@pids < 5) { + + push @pids, fork(); + die "Unable to fork: $!\n" + if ! defined $pids[-1]; + + if ($pids[-1]) { + next; + } + + if (@pids % 2) { + $schema->txn_do (sub { + my $depth = $schema->storage->transaction_depth; + die "$$(txn_do)unexpected txn depth $depth!" if $depth != 1; + $schema->resultset ('Artist')->create ({ name => "forking action $$"}); + }); + } + else { + my $guard = $schema->txn_scope_guard; + my $depth = $schema->storage->transaction_depth; + die "$$(scope_guard) unexpected txn depth $depth!" if $depth != 1; + $schema->resultset ('Artist')->create ({ name => "forking action $$"}); + $guard->commit; + } + + exit 0; + } + + return @pids; +} + my $fail_code = sub { my ($artist) = @_; $artist->create_related('cds', { @@ -316,12 +455,20 @@ undef $schema; # make sure the guard does not eat exceptions { - my $schema = DBICTest->init_schema(); + my $schema = DBICTest->init_schema; + + no strict 'refs'; + no warnings 'redefine'; + local *{DBIx::Class::Storage::DBI::txn_rollback} = sub { die 'die die my darling' }; + throws_ok (sub { my $guard = $schema->txn_scope_guard; $schema->resultset ('Artist')->create ({ name => 'bohhoo'}); - $schema->storage->disconnect; # this should freak out the guard rollback + # this should freak out the guard rollback + # but it won't work because DBD::SQLite is buggy + # instead just install a toxic rollback above + #$schema->storage->_dbh( $schema->storage->_dbh->clone ); die 'Deliberate exception'; }, qr/Deliberate exception.+Rollback failed/s); @@ -331,14 +478,21 @@ undef $schema; { my $schema = DBICTest->init_schema(); - # something is really confusing Test::Warn here, no time to debug + no strict 'refs'; + no warnings 'redefine'; + local *{DBIx::Class::Storage::DBI::txn_rollback} = sub { die 'die die my darling' }; + +#The warn from within a DESTROY callback freaks out Test::Warn, do it old-school =begin warnings_exist ( sub { my $guard = $schema->txn_scope_guard; $schema->resultset ('Artist')->create ({ name => 'bohhoo'}); - $schema->storage->disconnect; # this should freak out the guard rollback + # this should freak out the guard rollback + # but it won't work because DBD::SQLite is buggy + # instead just install a toxic rollback above + #$schema->storage->_dbh( $schema->storage->_dbh->clone ); }, [ qr/A DBIx::Class::Storage::TxnScopeGuard went out of scope without explicit commit or error. Rolling back./, @@ -348,6 +502,7 @@ undef $schema; ); =cut +# delete this once the above works properly (same test) my @want = ( qr/A DBIx::Class::Storage::TxnScopeGuard went out of scope without explicit commit or error. Rolling back./, qr/\*+ ROLLBACK FAILED\!\!\! \*+/, @@ -365,8 +520,6 @@ undef $schema; { my $guard = $schema->txn_scope_guard; $schema->resultset ('Artist')->create ({ name => 'bohhoo'}); - - $schema->storage->disconnect; # this should freak out the guard rollback } is (@w, 2, 'Both expected warnings found');