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

[Alex Vandiver]

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

- Log -----------------------------------------------------------------
commit c4b74bef625c0489bd3b715d6fd9017dba4c57cc
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..60ef4b3 100644
--- a/lib/RT/Record.pm
+++ b/lib/RT/Record.pm
@@ -1422,6 +1422,20 @@ Private function to create a new RT::Transaction object for this ticket update
 
 =cut
 
+sub LockForUpdate {
+    my $self = shift;
+    return if RT->Config->Get('DatabaseType') eq "SQLite";
+
+    my $pk = $self->_PrimaryKey;
+    my $id = @_ ? $_[0] : $self->$pk;
+    $self->_expire if $self->isa("DBIx::SearchBuilder::Record::Cachable");
+    return $self->_LoadFromSQL(
+        "SELECT * FROM ".$self->Table
+              ." WHERE $pk = ? FOR UPDATE",
+        $id,
+    );
+}
+
 sub _NewTransaction {
     my $self = shift;
     my %args = (
@@ -1441,6 +1455,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 +1506,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..5d59e5c
--- /dev/null
+++ b/t/ticket/race.t
@@ -0,0 +1,47 @@
+use strict;
+use warnings;
+
+use RT::Test tests => 1;
+
+use constant KIDS => 10;
+
+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" );

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