[Rt-commit] rt branch, 4.0/lock-transaction-updates, created. rt-4.0.5-93-gb99af56

[Alex Vandiver]

The branch, 4.0/lock-transaction-updates has been created
        at  b99af56fa6fed07f2ea055e0abed393ce3c6fb4a (commit)

- Log -----------------------------------------------------------------
commit b99af56fa6fed07f2ea055e0abed393ce3c6fb4a
Author: Alex Vandiver <alexmv at bestpractical.com>
Date:   Fri Mar 16 16:57:07 2012 -0400

    Lock transaction updates so scrips get a consistent snapshot
    
    Previously, nothing prevented multiple transactions from being run on
    the system concurrently, and making identical changes.  This could lead
    to multiple Corrrespondences, followed by multiple "Status changed from
    new to open" transactions.  Prevent this by always running
    ->_NewTransaction in a database transaction, and ensuring that it takes
    a write lock on the row before running scrips and purges the cache.
    This ensures a coherent and serial execution of scrips.

diff --git a/lib/RT/Record.pm b/lib/RT/Record.pm
index 29cff47..c116ee8 100644
--- a/lib/RT/Record.pm
+++ b/lib/RT/Record.pm
@@ -1413,8 +1413,35 @@ sub _DeleteLink {
 }
 
 
+=head1 LockForUpdate
 
+In a database transaction, gains an exclusive lock on the row, to
+prevent race conditions.  On SQLite, this is a "RESERVED" lock on the
+entire database.
 
+=cut
+
+sub LockForUpdate {
+    my $self = shift;
+
+    my $pk = $self->_PrimaryKey;
+    my $id = @_ ? $_[0] : $self->$pk;
+    $self->_expire if $self->isa("DBIx::SearchBuilder::Record::Cachable");
+    if (RT->Config->Get('DatabaseType') eq "SQLite") {
+        # SQLite does DB-level locking, upgrading the transaction to
+        # "RESERVED" on the first UPDATE/INSERT/DELETE.  Do a no-op
+        # UPDATE to force the upgade.
+        return RT->DatabaseHandle->dbh->do(
+            "UPDATE " .$self->Table.
+                " SET $pk = $pk WHERE 1 = 0");
+    } else {
+        return $self->_LoadFromSQL(
+            "SELECT * FROM ".$self->Table
+                ." WHERE $pk = ? FOR UPDATE",
+            $id,
+        );
+    }
+}
 
 =head2 _NewTransaction  PARAMHASH
 
@@ -1441,6 +1468,11 @@ sub _NewTransaction {
         @_
     );
 
+    my $in_txn = RT->DatabaseHandle->TransactionDepth;
+    RT->DatabaseHandle->BeginTransaction unless $in_txn;
+
+    $self->LockForUpdate;
+
     my $old_ref = $args{'OldReference'};
     my $new_ref = $args{'NewReference'};
     my $ref_type = $args{'ReferenceType'};
@@ -1487,6 +1519,9 @@ sub _NewTransaction {
     if ( RT->Config->Get('UseTransactionBatch') and $transaction ) {
 	    push @{$self->{_TransactionBatch}}, $trans if $args{'CommitScrips'};
     }
+
+    RT->DatabaseHandle->Commit unless $in_txn;
+
     return ( $transaction, $msg, $trans );
 }
 
diff --git a/lib/RT/Ticket.pm b/lib/RT/Ticket.pm
index 3f2e94c..79fa036 100644
--- a/lib/RT/Ticket.pm
+++ b/lib/RT/Ticket.pm
@@ -2095,14 +2095,16 @@ sub Comment {
     }
     $args{'NoteType'} = 'Comment';
 
+    $RT::Handle->BeginTransaction();
     if ($args{'DryRun'}) {
-        $RT::Handle->BeginTransaction();
         $args{'CommitScrips'} = 0;
     }
 
     my @results = $self->_RecordNote(%args);
     if ($args{'DryRun'}) {
         $RT::Handle->Rollback();
+    } else {
+        $RT::Handle->Commit();
     }
 
     return(@results);
@@ -2141,10 +2143,10 @@ sub Correspond {
              or ( $self->CurrentUserHasRight('ModifyTicket') ) ) {
         return ( 0, $self->loc("Permission Denied"), undef );
     }
+    $args{'NoteType'} = 'Correspond';
 
-    $args{'NoteType'} = 'Correspond'; 
+    $RT::Handle->BeginTransaction();
     if ($args{'DryRun'}) {
-        $RT::Handle->BeginTransaction();
         $args{'CommitScrips'} = 0;
     }
 
@@ -2161,6 +2163,8 @@ sub Correspond {
 
     if ($args{'DryRun'}) {
         $RT::Handle->Rollback();
+    } else {
+        $RT::Handle->Commit();
     }
 
     return (@results);
diff --git a/t/ticket/race.t b/t/ticket/race.t
new file mode 100644
index 0000000..aa1150e
--- /dev/null
+++ b/t/ticket/race.t
@@ -0,0 +1,51 @@
+use strict;
+use warnings;
+
+use RT::Test tests => 2;
+
+use constant KIDS => 50;
+
+my $id;
+
+{
+    my $t = RT::Ticket->new( RT->SystemUser );
+    ($id) = $t->Create(
+        Queue => "General",
+        Subject => "Race $$",
+    );
+}
+
+diag "Created ticket $id";
+RT->DatabaseHandle->Disconnect;
+
+my @kids;
+for (1..KIDS) {
+    if (my $pid = fork()) {
+        push @kids, $pid;
+        next;
+    }
+
+    # In the kid, load up the ticket and correspond
+    RT->ConnectToDatabase;
+    my $t = RT::Ticket->new( RT->SystemUser );
+    $t->Load( $id );
+    $t->Correspond( Content => "Correspondence from PID $$" );
+    undef $t;
+    exit 0;
+}
+
+
+diag "Forked @kids";
+waitpid $_, 0 for @kids;
+diag "All kids finished corresponding";
+
+RT->ConnectToDatabase;
+my $t = RT::Ticket->new( RT->SystemUser );
+$t->Load($id);
+my $txns = $t->Transactions;
+$txns->Limit( FIELD => 'Type', VALUE => 'Status' );
+is($txns->Count, 1, "Only one transaction change recorded" );
+
+$txns = $t->Transactions;
+$txns->Limit( FIELD => 'Type', VALUE => 'Correspond' );
+is($txns->Count, KIDS, "But all correspondences were recorded" );

-----------------------------------------------------------------------