[Rt-commit] rt branch, 4.4/previewscrips-race, created. rt-4.4.1-225-g8862bd9

Shawn Moore shawn at bestpractical.com
Fri Jan 13 17:20:24 EST 2017


The branch, 4.4/previewscrips-race has been created
        at  8862bd94d3ddc84ac858f36cff2a032e51fdf67c (commit)

- Log -----------------------------------------------------------------
commit db0a61d9186bd9b499d4d89ab1d406d0302e8be9
Author: Alex Vandiver <alex at chmrr.net>
Date:   Tue Jan 3 23:47:38 2017 -0800

    Add a test showing transaction deadlocks with concurrent ticket updates
    
    Ticket updates can deadlock due to their haphazard use of
    transactions; this test attempts to trigger such deadlocks.
    
    The real-world scenario is that PreviewScrips opens a transaction and
    (via ->_NewTransaction to record correspondence) calls ->LockForUpdate
    to lock the ticket object.  Meanwhile, the real update has been
    submitted, and calls ->SetOwner outside of a transaction.
    
    On PostgreSQL, the deadlock aborts and rolls back the SetOwner
    transaction; database integrity is preserved, but only one SetOwner
    succeeds:
    
        DBD::Pg::st execute failed: ERROR:  deadlock detected
        DETAIL:  Process 27554 waits for ShareLock on transaction 862271; blocked by process 27555.
        Process 27555 waits for ShareLock on transaction 862270; blocked by process 27554.
        HINT:  See server log for query details.
        CONTEXT:  while deleting tuple (0,104) in relation "cachedgroupmembers"
        RT::Handle=HASH(0x6409108) couldn't execute the query 'DELETE FROM CachedGroupMembers WHERE id=? '
                [stacktrace omitted]
        DBD::Pg::st execute failed: ERROR:  current transaction is aborted, commands ignored until end of transaction block
        RT::Handle=HASH(0x6409108) couldn't execute the query 'DELETE FROM GroupMembers WHERE id=? '
                [stacktrace omitted]
        Couldn't delete cached group submember 22
        Use of uninitialized value $_[1] in join or string at (eval 272) line 2.
        Rollback and commit are mixed while escaping nested transaction/warn
        not ok 5 - 27552 returned 1
    
        #   Failed test '27552 returned 1'
        #   at t/ticket/race.t line 106.
        #          got: '1'
        #     expected: '0'
        ok 6 - 27553 returned 0
        not ok 7 - Found two new SetWatcher transactions
    
        #   Failed test 'Found two new SetWatcher transactions'
        #   at t/ticket/race.t line 117.
        #          got: '1'
        #     expected: '2'
        ok 8 - Not the base owner
        ok 9 - GroupMembers agrees
        ok 10 - CachedGroupMembers agrees
    
    On MySQL, the outcome is worse -- deadlock does not abort the
    transaction, it merely causes that statement to fail, with a gentle
    suggestion to restart the transaction.  Because of bugs propagating
    this error up, this becomes a mostly-silent corruption of database
    consistency, with the denormalized 'Owner' column disagreeing with
    GroupMembers and CachedGroupMembers for the group:
    
        DBD::mysql::st execute failed: Deadlock found when trying to get lock; try restarting transaction
        RT::Handle=HASH(0x5f63cc8) couldn't execute the query 'DELETE FROM CachedGroupMembers WHERE id=? '
                [stacktrace omitted]
        ok 5 - 805 returned 0
        ok 6 - 806 returned 0
        ok 7 - Found two new SetWatcher transactions
        ok 8 - Not the base owner
        not ok 9 - GroupMembers agrees
    
        #   Failed test 'GroupMembers agrees'
        #   at t/ticket/race.t line 121.
        not ok 10 - CachedGroupMembers agrees
    
        #   Failed test 'CachedGroupMembers agrees'
        #   at t/ticket/race.t line 122.
    
    The following commits address these issues.

diff --git a/t/ticket/race.t b/t/ticket/race.t
index aa1150e..a167d6b 100644
--- a/t/ticket/race.t
+++ b/t/ticket/race.t
@@ -1,7 +1,8 @@
 use strict;
 use warnings;
 
-use RT::Test tests => 2;
+use RT::Test tests => undef;
+use Test::MockTime qw/set_fixed_time/;
 
 use constant KIDS => 50;
 
@@ -49,3 +50,76 @@ 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" );
+
+
+my @users;
+for my $n (0..2) {
+    push @users, RT::Test->load_or_create_user(
+        Name => "user_$n", Password => 'password',
+    )->id;
+}
+
+ok(RT::Test->add_rights({
+    Principal   => 'Privileged',
+    Right       => 'OwnTicket',
+}), "Granted OwnTicket");
+
+for my $round (1..10) {
+    my ($ok, $msg) = $t->SetOwner($users[0]);
+    ok $ok, "Set owner back to base";
+    my $last_txn = $t->Transactions->Last->id;
+    RT->DatabaseHandle->Disconnect;
+
+    diag "Round $round..\n";
+    @kids = ();
+    for my $n (1..2) {
+        if (my $pid = fork()) {
+            push @kids, $pid;
+            next;
+        }
+
+        set_fixed_time("2017-01-03T17:17:17Z");
+
+        # In the kid, load up the ticket and claim the owner
+        RT->ConnectToDatabase;
+        my $t = RT::Ticket->new( RT->SystemUser );
+        $t->Load( $id );
+
+        my ($ok, $msg);
+        if ($n == 1) {
+            $RT::Handle->BeginTransaction;
+            $t->LockForUpdate;
+            ($ok, $msg) = $t->SetOwner( $users[$n] );
+            undef $t;
+            $RT::Handle->Commit;
+        } else {
+            ($ok, $msg) = $t->SetOwner( $users[$n] );
+            undef $t;
+        }
+        exit(1 - $ok);
+    }
+
+    diag "Forked @kids";
+    for my $pid (@kids) {
+        waitpid $pid, 0;
+        my $ret = $? >> 8;
+        is $ret, 0, "$pid returned $ret";
+    }
+
+    RT->ConnectToDatabase;
+    # Flush the process-local cache and reload, since the changes
+    # happened in other processes
+    DBIx::SearchBuilder::Record::Cachable->FlushCache;
+    $t->Load( $id );
+    $txns = $t->Transactions;
+    $txns->Limit( FIELD => 'id', OPERATOR => '>', VALUE => $last_txn );
+    $txns->Limit( FIELD => 'Type', VALUE => 'SetWatcher' );
+    is $txns->Count, 2, "Found two new SetWatcher transactions";
+
+    my $winner = $t->Owner;
+    isnt $winner, $users[0], "Not the base owner";
+    ok $t->OwnerGroup->HasMember( $winner ), "GroupMembers agrees";
+    ok $t->OwnerGroup->HasMemberRecursively( $winner ), "CachedGroupMembers agrees";
+}
+
+done_testing;

commit 4701ed9e44dd96dd39dcf83211881479d18abc14
Author: Alex Vandiver <alex at chmrr.net>
Date:   Tue Jan 3 23:54:02 2017 -0800

    Propagate Delete failures up correctly
    
    3ed1adb3 made the ->Delete method return a tuple of ($ok, $msg) to
    line up with other parts of RT's API.  However, it did not adjust any
    existing callsites, all of which were of the form:
    
        my $del_err = $item_to_del->Delete();
    
    In scalar context, the comma operator returns its right-hand argument;
    as such, every call to RT::Record->Delete was deemed to be successful.
    
    Find every use of ->Delete in scalar context, and adjust it to expect
    a tuple.
    
    With respect to the previous commit, this causes failures of
    RT::CachedGroupMember->Delete to correctly propagate upward.  This
    aborts the SetOwner call on MySQL, and returns it to parity with
    PostgreSQL: both now lose one ->SetOwner call, but leave a consistent
    database.

diff --git a/lib/RT/Article.pm b/lib/RT/Article.pm
index c1922c4..0e223e5 100644
--- a/lib/RT/Article.pm
+++ b/lib/RT/Article.pm
@@ -420,8 +420,8 @@ sub DeleteTopic {
         ObjectType => ref($self)
     );
     if ( $t->Id ) {
-        my $del = $t->Delete;
-        unless ($del) {
+        my ($ok, $msg) = $t->Delete;
+        unless ($ok) {
             return (
                 undef,
                 $self->loc(
diff --git a/lib/RT/CachedGroupMember.pm b/lib/RT/CachedGroupMember.pm
index 63d9914..c310abe 100644
--- a/lib/RT/CachedGroupMember.pm
+++ b/lib/RT/CachedGroupMember.pm
@@ -183,16 +183,17 @@ sub Create {
 
 =head2 Delete
 
-Deletes the current CachedGroupMember from the group it's in and cascades 
-the delete to all submembers. This routine could be completely excised if
-mysql supported foreign keys with cascading deletes.
+Deletes the current CachedGroupMember from the group it's in and
+cascades the delete to all submembers. This routine could be
+completely excised if mysql supported foreign keys with cascading
+deletes.
 
-=cut 
+=cut
 
 sub Delete {
     my $self = shift;
 
-    
+
     my $member = $self->MemberObj();
     if ( $member->IsGroup ) {
         my $deletable = RT::CachedGroupMembers->new( $self->CurrentUser );
@@ -205,20 +206,17 @@ sub Delete {
                            VALUE    => $self->id );
 
         while ( my $kid = $deletable->Next ) {
-            my $kid_err = $kid->Delete();
-            unless ($kid_err) {
+            my ($ok, $msg) = $kid->Delete();
+            unless ($ok) {
                 $RT::Logger->error(
-                              "Couldn't delete CachedGroupMember " . $kid->Id );
-                return (undef);
+                    "Couldn't delete CachedGroupMember " . $kid->Id );
+                return ($ok, $msg);
             }
         }
     }
-    my $ret = $self->SUPER::Delete();
-    unless ($ret) {
-        $RT::Logger->error( "Couldn't delete CachedGroupMember " . $self->Id );
-        return (undef);
-    }
-    return $ret;
+    my ($ok, $msg) = $self->SUPER::Delete();
+    $RT::Logger->error( "Couldn't delete CachedGroupMember " . $self->Id ) unless $ok;
+    return ($ok, $msg);
 }
 
 
diff --git a/lib/RT/CustomField.pm b/lib/RT/CustomField.pm
index fba1edc..ae81702 100644
--- a/lib/RT/CustomField.pm
+++ b/lib/RT/CustomField.pm
@@ -695,11 +695,11 @@ sub DeleteValue {
         return (0, $self->loc("That is not a value for this custom field"));
     }
 
-    my $retval = $val_to_del->Delete;
-    unless ( $retval ) {
+    my ($ok, $msg) = $val_to_del->Delete;
+    unless ( $ok ) {
         return (0, $self->loc("Custom field value could not be deleted"));
     }
-    return ($retval, $self->loc("Custom field value deleted"));
+    return ($ok, $self->loc("Custom field value deleted"));
 }
 
 
@@ -1702,18 +1702,15 @@ sub RemoveFromObject {
         return ( 0, $self->loc("This custom field cannot be added to that object") );
     }
 
-    # XXX: Delete doesn't return anything
-    my $oid = $ocf->Delete;
+    my ($ok, $msg) = $ocf->Delete;
+    return ($ok, $msg) unless $ok;
 
-    my $msg;
     # If object has no id, it represents all objects
     if ($object->id) {
-        $msg = $self->loc( 'Removed custom field [_1] from [_2].', $self->Name, $object->Name );
+        return (1, $self->loc( 'Removed custom field [_1] from [_2].', $self->Name, $object->Name ) );
     } else {
-        $msg = $self->loc( 'Globally removed custom field [_1].', $self->Name );
+        return (1, $self->loc( 'Globally removed custom field [_1].', $self->Name ) );
     }
-
-    return ( $oid, $msg );
 }
 
 
@@ -1988,9 +1985,9 @@ sub DeleteValueForObject {
 
     # delete it
 
-    my $ret = $oldval->Delete();
-    unless ($ret) {
-        return(0, $self->loc("Custom field value could not be found"));
+    my ($ok, $msg) = $oldval->Delete();
+    unless ($ok) {
+        return(0, $self->loc("Custom field value could not be deleted"));
     }
     return($oldval->Id, $self->loc("Custom field value deleted"));
 }
diff --git a/lib/RT/Group.pm b/lib/RT/Group.pm
index 39990fb..cd9f86c 100644
--- a/lib/RT/Group.pm
+++ b/lib/RT/Group.pm
@@ -1180,10 +1180,10 @@ sub _DeleteMember {
 
     my $old_member = $member_obj->MemberId;
 
-    #Now that we've checked ACLs and sanity, delete the groupmember
-    my $val = $member_obj->Delete();
+    # Now that we've checked ACLs and sanity, delete the groupmember
+    my ($ok, $msg) = $member_obj->Delete();
 
-    unless ($val) {
+    unless ($ok) {
         $RT::Logger->debug("Failed to delete group ".$self->Id." member ". $member_id);
         return ( 0, $self->loc("Member not deleted" ));
     }
@@ -1227,7 +1227,7 @@ sub _DeleteMember {
         );
     }
 
-    return ( $val, $self->loc("Member deleted") );
+    return ( $ok, $self->loc("Member deleted") );
 }
 
 
diff --git a/lib/RT/GroupMember.pm b/lib/RT/GroupMember.pm
index 4de4592..c7a1b5c 100644
--- a/lib/RT/GroupMember.pm
+++ b/lib/RT/GroupMember.pm
@@ -325,24 +325,20 @@ sub Delete {
         VALUE    => $self->GroupObj->Id
     );
 
-
-
-
-
     while ( my $item_to_del = $cached_submembers->Next() ) {
-        my $del_err = $item_to_del->Delete();
-        unless ($del_err) {
-            $RT::Handle->Rollback();
+        my ($ok, $msg) = $item_to_del->Delete();
+        unless ($ok) {
             $RT::Logger->warning("Couldn't delete cached group submember ".$item_to_del->Id);
-            return (undef);
+            $RT::Handle->Rollback();
+            return ($ok, $msg);
         }
     }
 
-    my ($err, $msg) = $self->SUPER::Delete();
-    unless ($err) {
-            $RT::Logger->warning("Couldn't delete cached group submember ".$self->Id);
+    my ($ok, $msg) = $self->SUPER::Delete();
+    unless ($ok) {
+        $RT::Logger->warning("Couldn't delete cached group submember ".$self->Id);
         $RT::Handle->Rollback();
-        return (undef);
+        return ($ok, $msg);
     }
 
     #Clear the key cache. TODO someday we may want to just clear a little bit of the keycache space. 
@@ -350,7 +346,7 @@ sub Delete {
     RT::Principal->InvalidateACLCache();
 
     $RT::Handle->Commit();
-    return ($err);
+    return ($ok, $msg);
 
 }
 
diff --git a/lib/RT/Record.pm b/lib/RT/Record.pm
index 2aaf77d..cc06a76 100644
--- a/lib/RT/Record.pm
+++ b/lib/RT/Record.pm
@@ -127,9 +127,8 @@ sub Delete {
     if ($rv) {
         return ($rv, $self->loc("Object deleted"));
     } else {
-
-        return(0, $self->loc("Object could not be deleted"))
-    } 
+        return (0, $self->loc("Object could not be deleted"));
+    }
 }
 
 =head2 RecordType

commit 9c3931075686470e67e4f541c855c4b169c25ec7
Author: Alex Vandiver <alex at chmrr.net>
Date:   Wed Jan 4 03:17:53 2017 -0800

    Minor formatting fixes

diff --git a/lib/RT/Group.pm b/lib/RT/Group.pm
index cd9f86c..a7b2c61 100644
--- a/lib/RT/Group.pm
+++ b/lib/RT/Group.pm
@@ -1165,17 +1165,16 @@ sub _DeleteMember {
         @_,
     );
 
-
     my $member_obj =  RT::GroupMember->new( $self->CurrentUser );
-    
-    $member_obj->LoadByCols( MemberId  => $member_id,
-                             GroupId => $self->PrincipalId);
-
+    $member_obj->LoadByCols(
+        MemberId => $member_id,
+        GroupId  => $self->PrincipalId,
+    );
 
-    #If we couldn't load it, return undef.
+    # If we couldn't load it, return undef.
     unless ( $member_obj->Id() ) {
         $RT::Logger->debug("Group has no member with that id");
-        return ( 0,$self->loc( "Group has no such member" ));
+        return ( 0, $self->loc( "Group has no such member" ));
     }
 
     my $old_member = $member_obj->MemberId;
@@ -1195,12 +1194,12 @@ sub _DeleteMember {
         );
 
         if ($self->SingleMemberRoleGroup) {
-            # _AddMember creates the Set-Owner txn (for example) but we handle
-            # the SetWatcher-Owner txn below.
+            # _AddMember creates the Set-Owner txn (for example) but
+            # we handle the SetWatcher-Owner txn below.
             $self->_AddMember(
-                PrincipalId             => RT->Nobody->Id,
-                RecordTransaction       => 0,
-                RecordSetTransaction    => $args{RecordTransaction},
+                PrincipalId          => RT->Nobody->Id,
+                RecordTransaction    => 0,
+                RecordSetTransaction => $args{RecordTransaction},
             );
             $txn{Type}     = "SetWatcher";
             $txn{NewValue} = RT->Nobody->id;

commit 01b4ee497cd9506b0ec961097de5b7a45b376054
Author: Alex Vandiver <alex at chmrr.net>
Date:   Wed Jan 4 03:18:22 2017 -0800

    Remove a redundant variable

diff --git a/lib/RT/Group.pm b/lib/RT/Group.pm
index a7b2c61..78992a5 100644
--- a/lib/RT/Group.pm
+++ b/lib/RT/Group.pm
@@ -1177,8 +1177,6 @@ sub _DeleteMember {
         return ( 0, $self->loc( "Group has no such member" ));
     }
 
-    my $old_member = $member_obj->MemberId;
-
     # Now that we've checked ACLs and sanity, delete the groupmember
     my ($ok, $msg) = $member_obj->Delete();
 
@@ -1189,7 +1187,7 @@ sub _DeleteMember {
 
     if ($self->RoleClass) {
         my %txn = (
-            OldValue => $old_member,
+            OldValue => $member_id,
             Field    => $self->Name,
         );
 

commit efe128e2892b74bd5f421a1d954ab53cf0ce3775
Author: Alex Vandiver <alex at chmrr.net>
Date:   Wed Jan 4 03:19:17 2017 -0800

    Remove an unnecessary "safety" check
    
    RT::Principal->Load does SQL escaping properly, and SQL will error if
    passed anything that is not an integer.  No other method does this
    manner of verification.

diff --git a/lib/RT/Group.pm b/lib/RT/Group.pm
index 78992a5..555b5b6 100644
--- a/lib/RT/Group.pm
+++ b/lib/RT/Group.pm
@@ -929,11 +929,6 @@ sub _AddMember {
         return(0, $self->loc("Group not found"));
     }
 
-    unless ($new_member =~ /^\d+$/) {
-        $RT::Logger->crit("_AddMember called with a parameter that's not an integer.");
-    }
-
-
     my $new_member_obj = RT::Principal->new( $self->CurrentUser );
     $new_member_obj->Load($new_member);
 

commit 18a88f4779ff9639b66e18281b111cdfe73d42b8
Author: Alex Vandiver <alex at chmrr.net>
Date:   Wed Jan 4 03:21:45 2017 -0800

    Remove duplicated logging on ->Delete failure
    
    RT::CachedGroupMember->Delete already logs at the error level if it
    fails; there is no need for RT::GroupMember->Delete to additionally
    log a warning, and RT::Group->_DeleteMember to log at debug.
    
    Standardize on error-level as well for the case when the
    RT::GroupMember->SUPER::Delete() itself fails.

diff --git a/lib/RT/Group.pm b/lib/RT/Group.pm
index 555b5b6..37055ea 100644
--- a/lib/RT/Group.pm
+++ b/lib/RT/Group.pm
@@ -1174,11 +1174,7 @@ sub _DeleteMember {
 
     # Now that we've checked ACLs and sanity, delete the groupmember
     my ($ok, $msg) = $member_obj->Delete();
-
-    unless ($ok) {
-        $RT::Logger->debug("Failed to delete group ".$self->Id." member ". $member_id);
-        return ( 0, $self->loc("Member not deleted" ));
-    }
+    return ( 0, $self->loc("Member not deleted" )) unless $ok;
 
     if ($self->RoleClass) {
         my %txn = (
diff --git a/lib/RT/GroupMember.pm b/lib/RT/GroupMember.pm
index c7a1b5c..a9909e6 100644
--- a/lib/RT/GroupMember.pm
+++ b/lib/RT/GroupMember.pm
@@ -328,7 +328,6 @@ sub Delete {
     while ( my $item_to_del = $cached_submembers->Next() ) {
         my ($ok, $msg) = $item_to_del->Delete();
         unless ($ok) {
-            $RT::Logger->warning("Couldn't delete cached group submember ".$item_to_del->Id);
             $RT::Handle->Rollback();
             return ($ok, $msg);
         }
@@ -336,7 +335,7 @@ sub Delete {
 
     my ($ok, $msg) = $self->SUPER::Delete();
     unless ($ok) {
-        $RT::Logger->warning("Couldn't delete cached group submember ".$self->Id);
+        $RT::Logger->error("Couldn't delete GroupMember ".$self->Id);
         $RT::Handle->Rollback();
         return ($ok, $msg);
     }

commit 6c0cf77f50182b0cdaf56f7d7b199db7a8a94e66
Author: Alex Vandiver <alex at chmrr.net>
Date:   Wed Jan 4 03:24:59 2017 -0800

    Remove misleading or unhelpful comments

diff --git a/lib/RT/CachedGroupMember.pm b/lib/RT/CachedGroupMember.pm
index c310abe..eca6c68 100644
--- a/lib/RT/CachedGroupMember.pm
+++ b/lib/RT/CachedGroupMember.pm
@@ -184,9 +184,7 @@ sub Create {
 =head2 Delete
 
 Deletes the current CachedGroupMember from the group it's in and
-cascades the delete to all submembers. This routine could be
-completely excised if mysql supported foreign keys with cascading
-deletes.
+cascades the delete to all submembers.
 
 =cut
 
diff --git a/lib/RT/Group.pm b/lib/RT/Group.pm
index 37055ea..e6fe7dd 100644
--- a/lib/RT/Group.pm
+++ b/lib/RT/Group.pm
@@ -1146,11 +1146,7 @@ sub DeleteMember {
     $self->_DeleteMember($member_id, @_);
 }
 
-# A helper subroutine for DeleteMember that bypasses the ACL checks
-# this should _ONLY_ ever be called from Ticket/Queue  DeleteWatcher
-# when we want to deal with groups according to queue rights
-# In the dim future, this will all get factored out and life
-# will get better
+# A helper subroutine for DeleteMember that bypasses the ACL checks.
 
 sub _DeleteMember {
     my $self = shift;
diff --git a/lib/RT/GroupMember.pm b/lib/RT/GroupMember.pm
index a9909e6..581666b 100644
--- a/lib/RT/GroupMember.pm
+++ b/lib/RT/GroupMember.pm
@@ -292,8 +292,6 @@ sub _StashUser {
 Takes no arguments. deletes the currently loaded member from the 
 group in question.
 
-Expects to be called _outside_ a transaction
-
 =cut
 
 sub Delete {

commit 04aeb4cca1f0c1f4dfd86adfd18705e0e5068ebd
Author: Alex Vandiver <alex at chmrr.net>
Date:   Mon Jan 2 22:50:09 2017 -0800

    Take a real lock in SetOwner
    
    Ticket updates are done in piecemeal transactions, where individual
    pieces, at their own discretion, use transactions to ensure atomicity.
    Specifically, `_NewTransaction` and `SetOwner` choose to run within
    their own transactions.  The former uses `->LockForUpdate` (see
    b99af56f), the latter uses `->_SetLastUpdated` (added in 6061508e).
    
    c8a24b11 changed DryRun transactions (triggered asynchronously from
    the ticket update page) to do all of the above steps within a single
    database transaction, which is then rolled back.  Because of this
    overarching transaction, locks are held across different stages of the
    update -- which is a notable difference from the non-DryRun updates.
    
    Together, these can lead to deadlocks when updating owners and
    recording correspondence.  The timeline is as follows:
    
            Ticket/Display.html | PreviewScrips
        ------------------------+----------------------------------------------
         1.                     | Starting transaction in DryRun
                                |
         2. Entering ProcessUpdateMessage
         3. Starting transaction in _NewTransaction
         4. SELECT * FROM Tickets WHERE id = 1 FOR UPDATE
         5. INSERT INTO Transactions ...
         6. UPDATE Tickets SET LastUpdated=... WHERE id=1
         7. Committing transaction
                                |
         8.                     | Entering ProcessUpdateMessage
         9.                     | SELECT * FROM Tickets WHERE id = 1 FOR UPDATE
        10.                     | INSERT INTO Transactions ...
                                |
        11. Entering ProcessTicketBasics
        12. Starting transaction in SetOwner
        13. Doesn't update LastUpdated, as == now already
        14. DELETE FROM CachedGroupMembers WHERE id=...
        15. **BLOCKS**: UPDATE Tickets SET Owner=6 WHERE id=1
                                |
        16.                     | Entering ProcessTicketBasics
        17.                     | **BLOCKS**: DELETE FROM CachedGroupMembers WHERE id=...
    
    Step 9 acquires the lock on the Ticket row for the `PreviewScrips`
    request, and step 14 acquires the lock on the `CachedGroupMembers` row
    for the update.  The former blocks step 15, the latter blocks step 17.
    
    Note that the ticket update request had previously acquired the lock
    on the Ticket row, in step 4, but only for the duration of
    `ProcessUpdateMessage`, which it committed in step 7, thus releasing
    the lock.  The `PreviewScrips` request, in contrast, ran the same
    code, in steps 8-10, but did not release the lock because it was part
    of a larger nested transaction.
    
    The "lock" of `->_SetLastUpdated` (introduced in 6061508e) in step 13
    is in actuality only functional when there is a race between multiple
    different users.  While an update to the Ticket row would indeed serve
    to lock it, `->_SetLastUpdated` is only an update if the CurrentUser
    and timestamp differ from that stored in the database, as
    DBIx::SearchBuilder optimizes the `UPDATE` away if it matches the
    current database state.  When the issue is lock contention with
    yourself, the `LastUpdated` timestamp already matches from just
    previously (step 6), as well as the `LastUpdatedBy`, thus causing no
    row update and hence no lock.
    
    Switch to using `->LockForUpdate` instead of `->_SetLastUpdated`.
    This takes a reliable lock on the Ticket row; in the above example,
    this would block the ticket update at step 13, until PreviewScrips
    completed its work and rolled its transaction back.  This suffices to
    resolve the new tests in `t/ticket/race.t`.
    
    Fixes: I#32381

diff --git a/lib/RT/Ticket.pm b/lib/RT/Ticket.pm
index f62895c..1c646ca 100644
--- a/lib/RT/Ticket.pm
+++ b/lib/RT/Ticket.pm
@@ -1949,7 +1949,7 @@ sub SetOwner {
 
     $RT::Handle->BeginTransaction();
 
-    $self->_SetLastUpdated(); # lock the ticket
+    $self->LockForUpdate;
     $self->Load( $self->id ); # in case $self changed while waiting for lock
 
     my $OldOwnerObj = $self->OwnerObj;

commit 06925eda7483a43612d7e1db66cb6ba4a63b2a3c
Author: Alex Vandiver <alex at chmrr.net>
Date:   Wed Jan 4 02:47:36 2017 -0800

    Factor out, and use, an atomic transaction method on Tickets
    
    The previous commit introduced a pattern which is likely useful
    elsewhere -- that of updating a ticket in a manner that is guaranteed
    to be atomic.
    
    Factor out this behavior into a new method, which mirrors DryRun
    in some ways.  For additional safety when used with DryRun, the first
    SQL statement of both methods is the lock on the Ticket row; this
    forces any code within them to run serially.

diff --git a/lib/RT/Ticket.pm b/lib/RT/Ticket.pm
index 1c646ca..f3b2a7b 100644
--- a/lib/RT/Ticket.pm
+++ b/lib/RT/Ticket.pm
@@ -1626,9 +1626,72 @@ sub _RecordNote {
     return ( $Trans, $msg, $TransObj );
 }
 
+=head2 Atomic
+
+Takes one argument, a subroutine reference.  Starts a transaction,
+taking a write lock on this ticket object, and runs the subroutine in
+the context of that transaction.  Commits the transaction at the end
+of the block.  Returns whatever the subroutine returns.
+
+If the subroutine explicitly calls L<RT::Handle/Commit> or
+L<RT::Handle/Rollback>, this function respects that, and will skip is
+usual commit step.  If the subroutine dies, this function will abort
+the transaction (unless it is already aborted or committed, per
+above), and will re-die with the error.
+
+=cut
+
+sub Atomic {
+    my $self = shift;
+    my ($subref) = @_;
+    my $has_id = defined $self->id;
+    $RT::Handle->BeginTransaction;
+    my $depth = $RT::Handle->TransactionDepth;
+
+    $self->LockForUpdate if $has_id;
+    $self->Load( $self->id ) if $has_id;
+
+    my $context = wantarray;
+    my @ret;
+
+    local $@;
+    eval {
+        if ($context) {
+            @ret = $subref->();
+        } elsif (defined $context) {
+            @ret = scalar $subref->();
+        } else {
+            $subref->();
+        }
+    };
+    if ($@) {
+        $RT::Handle->Rollback if $RT::Handle->TransactionDepth == $depth;
+        die $@;
+    }
+
+    if ($RT::Handle->TransactionDepth == $depth) {
+        $self->ApplyTransactionBatch;
+        $RT::Handle->Commit;
+    }
+
+    return $context ? @ret : $ret[0];
+}
+
 
 =head2 DryRun
 
+Takes one argument, a subroutine reference.  Like L</Atomic>, starts a
+transaction and obtains a write lock on this ticket object, running
+the subroutine in the context of that transaction.
+
+In contrast to L</Atomic>, the transaction is B<always rolled back>.
+As such, the body of the function should not call L<RT::Handle/Commit>
+or L<RT::Handle/Rollback>, as that would break this method's ability
+to inspect the entire transaction.
+
+The return value of the subroutine reference is ignored.  Returns the
+set of L<RT::Transaction> objects that would have resulted from
+running the body of the transaction.
 
 =cut
 
@@ -1639,11 +1702,17 @@ sub DryRun {
 
     my @transactions;
 
+    my $has_id = defined $self->id;
+
     $RT::Handle->BeginTransaction();
     {
         # Getting nested "commit"s inside this rollback is fine
         local %DBIx::SearchBuilder::Handle::TRANSROLLBACK;
         local $self->{DryRun} = \@transactions;
+
+        # Get a write lock for this whole transaction
+        $self->LockForUpdate if $has_id;
+
         eval { $subref->() };
         warn "Error is $@" if $@;
         $self->ApplyTransactionBatch;
@@ -1947,41 +2016,32 @@ sub SetOwner {
     my $NewOwner = shift;
     my $Type     = shift || "Set";
 
-    $RT::Handle->BeginTransaction();
-
-    $self->LockForUpdate;
-    $self->Load( $self->id ); # in case $self changed while waiting for lock
+    return $self->Atomic(sub{
 
-    my $OldOwnerObj = $self->OwnerObj;
+        my $OldOwnerObj = $self->OwnerObj;
 
-    my $NewOwnerObj = RT::User->new( $self->CurrentUser );
-    $NewOwnerObj->Load( $NewOwner );
+        my $NewOwnerObj = RT::User->new( $self->CurrentUser );
+        $NewOwnerObj->Load( $NewOwner );
 
-    my ( $val, $msg ) = $self->CurrentUserCanSetOwner(
-                            NewOwnerObj => $NewOwnerObj,
-                            Type        => $Type );
+        my ( $val, $msg ) = $self->CurrentUserCanSetOwner(
+                                NewOwnerObj => $NewOwnerObj,
+                                Type        => $Type );
+        return ( $val, $msg ) unless $val;
 
-    unless ($val) {
-        $RT::Handle->Rollback();
-        return ( $val, $msg );
-    }
-
-    ($val, $msg ) = $self->OwnerGroup->_AddMember(
-        PrincipalId       => $NewOwnerObj->PrincipalId,
-        InsideTransaction => 1,
-        Object            => $self,
-    );
-    unless ($val) {
-        $RT::Handle->Rollback;
-        return ( 0, $self->loc("Could not change owner: [_1]", $msg) );
-    }
-
-    $msg = $self->loc( "Owner changed from [_1] to [_2]",
-                       $OldOwnerObj->Name, $NewOwnerObj->Name );
-
-    $RT::Handle->Commit();
+        ($val, $msg ) = $self->OwnerGroup->_AddMember(
+            PrincipalId       => $NewOwnerObj->PrincipalId,
+            InsideTransaction => 1,
+            Object            => $self,
+        );
+        unless ($val) {
+            $RT::Handle->Rollback;
+            return ( 0, $self->loc("Could not change owner: [_1]", $msg) );
+        }
 
-    return ( $val, $msg );
+        $msg = $self->loc( "Owner changed from [_1] to [_2]",
+                           $OldOwnerObj->Name, $NewOwnerObj->Name );
+        return ($val, $msg);
+    });
 }
 
 =head2 CurrentUserCanSetOwner

commit 8862bd94d3ddc84ac858f36cff2a032e51fdf67c
Author: Alex Vandiver <alex at chmrr.net>
Date:   Wed Jan 4 02:34:17 2017 -0800

    Switch to using ->Atomic for all ticket updates via the UI
    
    This reduces the chance of other deadlocks that may be lurking
    elsewhere, by serializing all updates to a given ticket.  This
    replaces an existing pattern of simply calling
    `ApplyTransactionBatch`, introduced in b5aedb8d and bd456fc5.

diff --git a/lib/RT/Ticket.pm b/lib/RT/Ticket.pm
index f3b2a7b..e00a80a 100644
--- a/lib/RT/Ticket.pm
+++ b/lib/RT/Ticket.pm
@@ -1639,6 +1639,10 @@ usual commit step.  If the subroutine dies, this function will abort
 the transaction (unless it is already aborted or committed, per
 above), and will re-die with the error.
 
+This method should be used to lock, and operate atomically on, all
+ticket changes via the UI
+(e.g. L<RT::Interface::Web/ProcessTicketBasics>).
+
 =cut
 
 sub Atomic {
diff --git a/share/html/Search/Bulk.html b/share/html/Search/Bulk.html
index 2d054d3..a8ddde8 100644
--- a/share/html/Search/Bulk.html
+++ b/share/html/Search/Bulk.html
@@ -285,10 +285,13 @@ my @linkresults;
 $Tickets->RedoSearch();
 
 unless ( $ARGS{'AddMoreAttach'} ) {
+    $RT::Handle->BeginTransaction;
     while ( my $Ticket = $Tickets->Next ) {
         my $tid = $Ticket->id;
         next unless grep $tid == $_, @UpdateTicket;
 
+        $Ticket->LockForUpdate;
+
         #Update the links
         $ARGS{'id'} = $Ticket->id;
 
@@ -324,6 +327,7 @@ unless ( $ARGS{'AddMoreAttach'} ) {
 
         @results = ( @results, @tempresults );
     }
+    $RT::Handle->Commit;
 
     delete $session{'Attachments'}{ $ARGS{'Token'} };
 
diff --git a/share/html/SelfService/Display.html b/share/html/SelfService/Display.html
index ad4105c..e01f0c5 100644
--- a/share/html/SelfService/Display.html
+++ b/share/html/SelfService/Display.html
@@ -122,22 +122,24 @@ if ( ($id[0]||'') eq 'new' ) {
 else {
     $Ticket = LoadTicket($ARGS{'id'});
 
-    push @results, ProcessUpdateMessage(
-        ARGSRef   => \%ARGS,
-        TicketObj => $Ticket
-    );
-
-    my @cfupdates = ProcessObjectCustomFieldUpdates(Object => $Ticket, ARGSRef => \%ARGS);
-    push (@results, @cfupdates);
-
-    #Update the status
-    if (    ( defined $ARGS{'Status'} )
-        and $ARGS{'Status'}
-        and ( $ARGS{'Status'} ne $Ticket->Status ) )
-    {
-        my ($code, $msg) = $Ticket->SetStatus( $ARGS{'Status'} );
-        push @results, "$msg";
-    }
+    $Ticket->Atomic(sub{
+        push @results, ProcessUpdateMessage(
+            ARGSRef   => \%ARGS,
+            TicketObj => $Ticket
+        );
+
+        my @cfupdates = ProcessObjectCustomFieldUpdates(Object => $Ticket, ARGSRef => \%ARGS);
+        push (@results, @cfupdates);
+
+        #Update the status
+        if (    ( defined $ARGS{'Status'} )
+            and $ARGS{'Status'}
+            and ( $ARGS{'Status'} ne $Ticket->Status ) )
+        {
+            my ($code, $msg) = $Ticket->SetStatus( $ARGS{'Status'} );
+            push @results, "$msg";
+        }
+    });
 
 }
 
diff --git a/share/html/Ticket/Display.html b/share/html/Ticket/Display.html
index ecc208a..846e441 100644
--- a/share/html/Ticket/Display.html
+++ b/share/html/Ticket/Display.html
@@ -160,12 +160,13 @@ if ($ARGS{'id'} eq 'new') {
 
     my $SkipProcessing;
 
-    $m->callback( CallbackName => 'BeforeProcessArguments',
-        TicketObj => $TicketObj,
-        ActionsRef => \@Actions, ARGSRef => \%ARGS,
-        SkipProcessing => \$SkipProcessing );
+    $TicketObj->Atomic(sub{
+        $m->callback( CallbackName => 'BeforeProcessArguments',
+            TicketObj => $TicketObj,
+            ActionsRef => \@Actions, ARGSRef => \%ARGS,
+            SkipProcessing => \$SkipProcessing );
+        return if $SkipProcessing;
 
-    if ( !$SkipProcessing ) {
         if ( defined $ARGS{'Action'} ) {
             if ($ARGS{'Action'} =~ /^(Steal|Delete|Take|SetTold)$/) {
                 my $action = $1;
@@ -174,11 +175,11 @@ if ($ARGS{'id'} eq 'new') {
             }
         }
 
-        $m->callback(CallbackName => 'ProcessArguments', 
-                Ticket => $TicketObj, 
-                ARGSRef => \%ARGS, 
+        $m->callback(CallbackName => 'ProcessArguments',
+                Ticket => $TicketObj,
+                ARGSRef => \%ARGS,
                 Actions => \@Actions);
-        
+
         push @Actions, ProcessUpdateMessage(
             ARGSRef   => \%ARGS,
             Actions   => \@Actions,
@@ -192,7 +193,8 @@ if ($ARGS{'id'} eq 'new') {
         push @Actions, ProcessTicketDates(   ARGSRef => \%ARGS, TicketObj => $TicketObj );
         push @Actions, ProcessObjectCustomFieldUpdates(ARGSRef => \%ARGS, TicketObj => $TicketObj );
         push @Actions, ProcessTicketReminders( ARGSRef => \%ARGS, TicketObj => $TicketObj );
-
+    });
+    if ( !$SkipProcessing ) {
         unless ($TicketObj->CurrentUserHasRight('ShowTicket')) {
             if (@Actions) {
                 Abort("A change was applied successfully, but you no longer have permissions to view the ticket", Actions => \@Actions);
diff --git a/share/html/Ticket/Modify.html b/share/html/Ticket/Modify.html
index db9f3a0..665a663 100644
--- a/share/html/Ticket/Modify.html
+++ b/share/html/Ticket/Modify.html
@@ -94,13 +94,13 @@ $m->callback( TicketObj => $TicketObj, CustomFields => $CustomFields, ARGSRef =>
 }
 
 unless ($skip_update) {
-    push @results, ProcessTicketBasics(TicketObj => $TicketObj, ARGSRef => \%ARGS);
-    push @results, ProcessTicketWatchers(TicketObj => $TicketObj, ARGSRef => \%ARGS);
-    push @results, ProcessObjectCustomFieldUpdates(Object => $TicketObj, ARGSRef => \%ARGS);
-    $m->callback( CallbackName => 'ProcessUpdates', TicketObj => $TicketObj,
-                  ARGSRef => \%ARGS, Results => \@results );
-
-    $TicketObj->ApplyTransactionBatch;
+    $TicketObj->Atomic(sub{
+        push @results, ProcessTicketBasics(TicketObj => $TicketObj, ARGSRef => \%ARGS);
+        push @results, ProcessTicketWatchers(TicketObj => $TicketObj, ARGSRef => \%ARGS);
+        push @results, ProcessObjectCustomFieldUpdates(Object => $TicketObj, ARGSRef => \%ARGS);
+        $m->callback( CallbackName => 'ProcessUpdates', TicketObj => $TicketObj,
+                      ARGSRef => \%ARGS, Results => \@results );
+    });
 
     MaybeRedirectForResults(
         Actions   => \@results,
diff --git a/share/html/Ticket/ModifyAll.html b/share/html/Ticket/ModifyAll.html
index 7e9c4ef..8217313 100644
--- a/share/html/Ticket/ModifyAll.html
+++ b/share/html/Ticket/ModifyAll.html
@@ -191,14 +191,14 @@ if ( ref ($ARGS{'Owner'} )) {
 }
 
 unless ($skip_update or $OnlySearchForPeople or $OnlySearchForGroup or $ARGS{'AddMoreAttach'} ) {
-    push @results, ProcessTicketWatchers( TicketObj => $Ticket, ARGSRef => \%ARGS);
-    push @results, ProcessObjectCustomFieldUpdates( Object => $Ticket, ARGSRef => \%ARGS);
-    push @results, ProcessTicketDates( TicketObj => $Ticket, ARGSRef => \%ARGS);
-    push @results, ProcessUpdateMessage( TicketObj => $Ticket, ARGSRef=>\%ARGS );
-    push @results, ProcessTicketBasics( TicketObj => $Ticket, ARGSRef => \%ARGS );
-    push @results, ProcessTicketLinks( TicketObj => $Ticket, ARGSRef => \%ARGS);
-
-    $Ticket->ApplyTransactionBatch;
+    $Ticket->Atomic(sub {
+        push @results, ProcessTicketWatchers( TicketObj => $Ticket, ARGSRef => \%ARGS);
+        push @results, ProcessObjectCustomFieldUpdates( Object => $Ticket, ARGSRef => \%ARGS);
+        push @results, ProcessTicketDates( TicketObj => $Ticket, ARGSRef => \%ARGS);
+        push @results, ProcessUpdateMessage( TicketObj => $Ticket, ARGSRef=>\%ARGS );
+        push @results, ProcessTicketBasics( TicketObj => $Ticket, ARGSRef => \%ARGS );
+        push @results, ProcessTicketLinks( TicketObj => $Ticket, ARGSRef => \%ARGS);
+    });
 
     MaybeRedirectForResults(
         Actions   => \@results,
diff --git a/share/html/Ticket/ModifyDates.html b/share/html/Ticket/ModifyDates.html
index ded54f7..dfa24e7 100644
--- a/share/html/Ticket/ModifyDates.html
+++ b/share/html/Ticket/ModifyDates.html
@@ -67,9 +67,10 @@
 my $TicketObj = LoadTicket($id);
 my @results;
 $m->callback( TicketObj => $TicketObj, ARGSRef => \%ARGS, results => \@results );
-push @results, ProcessTicketDates( TicketObj => $TicketObj, ARGSRef => \%ARGS);
-push @results, ProcessObjectCustomFieldUpdates(Object => $TicketObj, ARGSRef => \%ARGS);
-$TicketObj->ApplyTransactionBatch;
+$TicketObj->Atomic(sub{
+    push @results, ProcessTicketDates( TicketObj => $TicketObj, ARGSRef => \%ARGS);
+    push @results, ProcessObjectCustomFieldUpdates(Object => $TicketObj, ARGSRef => \%ARGS);
+});
 
 $TicketObj->CurrentUser->AddRecentlyViewedTicket($TicketObj);
 
diff --git a/share/html/Ticket/ModifyLinks.html b/share/html/Ticket/ModifyLinks.html
index 3d618cf..af37134 100644
--- a/share/html/Ticket/ModifyLinks.html
+++ b/share/html/Ticket/ModifyLinks.html
@@ -72,11 +72,12 @@
 <%INIT>
 my $Ticket = LoadTicket($id);
 
-my @results;  
-$m->callback( TicketObj => $Ticket, ARGSRef => \%ARGS, Results => \@results );
-push @results, ProcessTicketLinks( TicketObj => $Ticket, ARGSRef => \%ARGS );
-push @results, ProcessObjectCustomFieldUpdates( TicketObj => $Ticket, ARGSRef => \%ARGS );
-$Ticket->ApplyTransactionBatch;
+my @results;
+$Ticket->Atomic(sub{
+    $m->callback( TicketObj => $Ticket, ARGSRef => \%ARGS, Results => \@results );
+    push @results, ProcessTicketLinks( TicketObj => $Ticket, ARGSRef => \%ARGS );
+    push @results, ProcessObjectCustomFieldUpdates( TicketObj => $Ticket, ARGSRef => \%ARGS );
+});
 
 MaybeRedirectForResults(
     Actions     => \@results,
diff --git a/share/html/Ticket/ModifyPeople.html b/share/html/Ticket/ModifyPeople.html
index b09fb2d..6e5cbc2 100644
--- a/share/html/Ticket/ModifyPeople.html
+++ b/share/html/Ticket/ModifyPeople.html
@@ -106,11 +106,11 @@ $Ticket->SquelchMailTo($_)
 
 # if we're trying to search for watchers and nothing else
 unless ($OnlySearchForPeople or $OnlySearchForGroup) {
-    push @results, ProcessTicketBasics( TicketObj => $Ticket, ARGSRef => \%ARGS);
-    push @results, ProcessTicketWatchers( TicketObj => $Ticket, ARGSRef => \%ARGS);
-    push @results, ProcessObjectCustomFieldUpdates( TicketObj => $Ticket, ARGSRef => \%ARGS );
-
-    $Ticket->ApplyTransactionBatch;
+    $Ticket->Atomic(sub{
+        push @results, ProcessTicketBasics( TicketObj => $Ticket, ARGSRef => \%ARGS);
+        push @results, ProcessTicketWatchers( TicketObj => $Ticket, ARGSRef => \%ARGS);
+        push @results, ProcessObjectCustomFieldUpdates( TicketObj => $Ticket, ARGSRef => \%ARGS );
+    });
 }
 
 # Use the ticket's scrips to figure out the new list of recipients.
diff --git a/share/html/Ticket/Reminders.html b/share/html/Ticket/Reminders.html
index a599ee6..9ff8ab8 100644
--- a/share/html/Ticket/Reminders.html
+++ b/share/html/Ticket/Reminders.html
@@ -67,7 +67,9 @@
 <%INIT>
 my $Ticket = LoadTicket($id);
 
-my @actions = ProcessTicketReminders( TicketObj => $Ticket, ARGSRef => \%ARGS );
+my @actions = $Ticket->Atomic(sub{
+    ProcessTicketReminders( TicketObj => $Ticket, ARGSRef => \%ARGS );
+});
 
 $Ticket->CurrentUser->AddRecentlyViewedTicket($Ticket);
 
diff --git a/share/html/m/ticket/show b/share/html/m/ticket/show
index 477ae74..b686453 100644
--- a/share/html/m/ticket/show
+++ b/share/html/m/ticket/show
@@ -73,39 +73,42 @@ if ($ARGS{'id'} eq 'new') {
     unless ( $Ticket->CurrentUserHasRight('ShowTicket') ) {
         Abort("No permission to view newly created ticket #".$Ticket->id.".");
     }
-} else { 
+} else {
     $Ticket ||= LoadTicket($ARGS{'id'});
 
-    $m->callback( CallbackName => 'BeforeProcessArguments',
-        TicketObj => $Ticket,
-        ActionsRef => \@Actions, ARGSRef => \%ARGS );
-    if ( defined $ARGS{'Action'} ) {
-        if ($ARGS{'Action'} =~ /^(Steal|Delete|Take|SetTold)$/) {
-            my $action = $1;
-            my ($res, $msg) = $Ticket->$action();
-            push(@Actions, $msg);
+    $Ticket->Atomic(sub{
+        $m->callback( CallbackName => 'BeforeProcessArguments',
+            TicketObj => $Ticket,
+            ActionsRef => \@Actions, ARGSRef => \%ARGS );
+
+        if ( defined $ARGS{'Action'} ) {
+            if ($ARGS{'Action'} =~ /^(Steal|Delete|Take|SetTold)$/) {
+                my $action = $1;
+                my ($res, $msg) = $Ticket->$action();
+                push(@Actions, $msg);
+            }
         }
-    }
-
-    $m->callback(CallbackName => 'ProcessArguments', 
-            Ticket => $Ticket, 
-            ARGSRef => \%ARGS, 
-            Actions => \@Actions);
-    
-    push @Actions,
-        ProcessUpdateMessage(
-        ARGSRef   => \%ARGS,
-        Actions   => \@Actions,
-        TicketObj => $Ticket,
-        );
 
-    #Process status updates
-    push @Actions, ProcessTicketWatchers(ARGSRef => \%ARGS, TicketObj => $Ticket );
-    push @Actions, ProcessTicketBasics(  ARGSRef => \%ARGS, TicketObj => $Ticket );
-    push @Actions, ProcessTicketLinks(   ARGSRef => \%ARGS, TicketObj => $Ticket );
-    push @Actions, ProcessTicketDates(   ARGSRef => \%ARGS, TicketObj => $Ticket );
-    push @Actions, ProcessObjectCustomFieldUpdates(ARGSRef => \%ARGS, TicketObj => $Ticket );
-    push @Actions, ProcessTicketReminders( ARGSRef => \%ARGS, TicketObj => $Ticket );
+        $m->callback(CallbackName => 'ProcessArguments',
+                Ticket => $Ticket,
+                ARGSRef => \%ARGS,
+                Actions => \@Actions);
+
+        push @Actions,
+            ProcessUpdateMessage(
+            ARGSRef   => \%ARGS,
+            Actions   => \@Actions,
+            TicketObj => $Ticket,
+            );
+
+        #Process status updates
+        push @Actions, ProcessTicketWatchers(ARGSRef => \%ARGS, TicketObj => $Ticket );
+        push @Actions, ProcessTicketBasics(  ARGSRef => \%ARGS, TicketObj => $Ticket );
+        push @Actions, ProcessTicketLinks(   ARGSRef => \%ARGS, TicketObj => $Ticket );
+        push @Actions, ProcessTicketDates(   ARGSRef => \%ARGS, TicketObj => $Ticket );
+        push @Actions, ProcessObjectCustomFieldUpdates(ARGSRef => \%ARGS, TicketObj => $Ticket );
+        push @Actions, ProcessTicketReminders( ARGSRef => \%ARGS, TicketObj => $Ticket );
+    });
 
     unless ($Ticket->CurrentUserHasRight('ShowTicket')) {
         if (@Actions) {

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


More information about the rt-commit mailing list