[Rt-commit] rt branch, 4.4/previewscrips-race, created. rt-4.4.1-225-gc6859e3
Alex Vandiver
alexmv at bestpractical.com
Wed Jan 4 07:45:25 EST 2017
The branch, 4.4/previewscrips-race has been created
at c6859e39029c7f3b8a652f74f5d46c0ce178581d (commit)
- Log -----------------------------------------------------------------
commit e36364c5c8c30c5d0970f46ba344827e2a8260cb
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 30185284dcbfe6e4f3016023018a7bb3aa359e52
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 01dca64f787061a385186ed440e031ba7d669517
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 65fd2b5492f6f6ee3f6fe76271ed2289c8a271a7
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 6283b7d0aeeb8a1e55639589b60d55ff0b379ed6
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 a5427087d6ce4d2b6986992fdf436af7fc77ea0b
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 92edf4ac6f492790546a4ee330dc171bbc9293b4
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 6c852c3178d00e77b0fee37bb3cc96379992140d
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 an 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`.
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 3367f728e604914b7e9390f8e23f1c6b4cdc2f6a
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 running a ticket in a manner that is guaranteed
to be atomic.
Factor out this behavior into a new method, which is mirror to 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 c6859e39029c7f3b8a652f74f5d46c0ce178581d
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