recaptime.dev / phorge
@recaptime-dev's working patches + fork for Phorge, a community fork of Phabricator. (Upstream dev and stable branches are at upstream/main and upstream/stable respectively.) hq.recaptime.dev/wiki/Phorge
phorge phabricator
1
fork

Configure Feed

Select the types of activity you want to include in your feed.

When applying transactions, acquire a read lock sooner

Summary:
Depends on D20461. Ref T13276. Ref T13054.

Currently, we acquire the transaction read lock after populating "old values" in transactions and filtering transactions with no effect.

This isn't early enough to prevent all weird chaotic races: if two processes try to apply a "close revision" transaction at the same time, this can happen:

```
PROCESS A PROCESS B
Old Value = Open Old Value = Open
Transaction OK: Yes Transaction OK: Yes
Acquire Read Lock Acquire Read Lock
Got Read Lock! Wait...
Apply Transactions Wait...
New Value = Closed Wait...
Release Lock Wait...
Got Read Lock!
Apply Transactions
New Value = Closed
Release Lock
```

That's not great: both processes apply an "Open -> Closed" transaction since this was a valid transaction from the viewpoint of each process when it did the checks.

We actually want this:

```
PROCESS A PROCESS B
Acquire Read Lock Acquire Read Lock
Got Read Lock! Wait...
Old Value = Open Wait...
Transaction OK: Yes Wait...
Apply Transactions Wait...
New Value = Closed Wait...
Release Lock Wait...
Got Read Lock!
>>> Old Value = Closed
>>> Transaction Has No Effect!
>>> Do Nothing / Abort
Release Lock
```

Move the "lock" part up so we do that.

This may cause some kind of weird second-order effects, but T13054 went through pretty cleanly and we have to do this to get correct behavior, so we can survive those if/when they arise.

Test Plan:
- Added a `sleep(10)` before the lock.
- Ran `bin/repository message --reparse X` in two console windows, where X is a commit that closes revision Y and Y is open.
- Before patch: both windows closed the revision and added duplicate transactions.
- After patch: only one of the processes had an effect.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: jmeador

Maniphest Tasks: T13276, T13054

Differential Revision: https://secure.phabricator.com/D20462

epriestley b3b1cc64 42c02557

+92 -77
+92 -77
src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php
··· 977 977 PhabricatorLiskDAO $object, 978 978 array $xactions) { 979 979 980 - $this->object = $object; 981 - $this->xactions = $xactions; 982 - $this->isNewObject = ($object->getPHID() === null); 980 + $is_new = ($object->getPHID() === null); 981 + $this->isNewObject = $is_new; 983 982 984 - $this->validateEditParameters($object, $xactions); 985 - $xactions = $this->newMFATransactions($object, $xactions); 983 + $is_preview = $this->getIsPreview(); 984 + $read_locking = false; 985 + $transaction_open = false; 986 986 987 - $actor = $this->requireActor(); 987 + // If we're attempting to apply transactions, lock and reload the object 988 + // before we go anywhere. If we don't do this at the very beginning, we 989 + // may be looking at an older version of the object when we populate and 990 + // filter the transactions. See PHI1165 for an example. 988 991 989 - // NOTE: Some transaction expansion requires that the edited object be 990 - // attached. 991 - foreach ($xactions as $xaction) { 992 - $xaction->attachObject($object); 993 - $xaction->attachViewer($actor); 994 - } 992 + if (!$is_preview) { 993 + if (!$is_new) { 994 + $this->buildOldRecipientLists($object, $xactions); 995 + 996 + $object->openTransaction(); 997 + $transaction_open = true; 995 998 996 - $xactions = $this->expandTransactions($object, $xactions); 997 - $xactions = $this->expandSupportTransactions($object, $xactions); 998 - $xactions = $this->combineTransactions($xactions); 999 + $object->beginReadLocking(); 1000 + $read_locking = true; 999 1001 1000 - foreach ($xactions as $xaction) { 1001 - $xaction = $this->populateTransaction($object, $xaction); 1002 + $object->reload(); 1003 + } 1002 1004 } 1003 1005 1004 - $is_preview = $this->getIsPreview(); 1005 - $read_locking = false; 1006 - $transaction_open = false; 1006 + try { 1007 + $this->object = $object; 1008 + $this->xactions = $xactions; 1007 1009 1008 - if (!$is_preview) { 1009 - $errors = array(); 1010 - $type_map = mgroup($xactions, 'getTransactionType'); 1011 - foreach ($this->getTransactionTypes() as $type) { 1012 - $type_xactions = idx($type_map, $type, array()); 1013 - $errors[] = $this->validateTransaction($object, $type, $type_xactions); 1014 - } 1010 + $this->validateEditParameters($object, $xactions); 1011 + $xactions = $this->newMFATransactions($object, $xactions); 1015 1012 1016 - $errors[] = $this->validateAllTransactions($object, $xactions); 1017 - $errors[] = $this->validateTransactionsWithExtensions($object, $xactions); 1018 - $errors = array_mergev($errors); 1013 + $actor = $this->requireActor(); 1019 1014 1020 - $continue_on_missing = $this->getContinueOnMissingFields(); 1021 - foreach ($errors as $key => $error) { 1022 - if ($continue_on_missing && $error->getIsMissingFieldError()) { 1023 - unset($errors[$key]); 1024 - } 1015 + // NOTE: Some transaction expansion requires that the edited object be 1016 + // attached. 1017 + foreach ($xactions as $xaction) { 1018 + $xaction->attachObject($object); 1019 + $xaction->attachViewer($actor); 1025 1020 } 1026 1021 1027 - if ($errors) { 1028 - throw new PhabricatorApplicationTransactionValidationException($errors); 1022 + $xactions = $this->expandTransactions($object, $xactions); 1023 + $xactions = $this->expandSupportTransactions($object, $xactions); 1024 + $xactions = $this->combineTransactions($xactions); 1025 + 1026 + foreach ($xactions as $xaction) { 1027 + $xaction = $this->populateTransaction($object, $xaction); 1029 1028 } 1030 1029 1031 - if ($this->raiseWarnings) { 1032 - $warnings = array(); 1033 - foreach ($xactions as $xaction) { 1034 - if ($this->hasWarnings($object, $xaction)) { 1035 - $warnings[] = $xaction; 1036 - } 1030 + if (!$is_preview) { 1031 + $errors = array(); 1032 + $type_map = mgroup($xactions, 'getTransactionType'); 1033 + foreach ($this->getTransactionTypes() as $type) { 1034 + $type_xactions = idx($type_map, $type, array()); 1035 + $errors[] = $this->validateTransaction( 1036 + $object, 1037 + $type, 1038 + $type_xactions); 1037 1039 } 1038 - if ($warnings) { 1039 - throw new PhabricatorApplicationTransactionWarningException( 1040 - $warnings); 1041 - } 1042 - } 1043 - } 1044 1040 1045 - foreach ($xactions as $xaction) { 1046 - $this->adjustTransactionValues($object, $xaction); 1047 - } 1041 + $errors[] = $this->validateAllTransactions($object, $xactions); 1042 + $errors[] = $this->validateTransactionsWithExtensions( 1043 + $object, 1044 + $xactions); 1045 + $errors = array_mergev($errors); 1048 1046 1049 - // Now that we've merged and combined transactions, check for required 1050 - // capabilities. Note that we're doing this before filtering 1051 - // transactions: if you try to apply an edit which you do not have 1052 - // permission to apply, we want to give you a permissions error even 1053 - // if the edit would have no effect. 1054 - $this->applyCapabilityChecks($object, $xactions); 1047 + $continue_on_missing = $this->getContinueOnMissingFields(); 1048 + foreach ($errors as $key => $error) { 1049 + if ($continue_on_missing && $error->getIsMissingFieldError()) { 1050 + unset($errors[$key]); 1051 + } 1052 + } 1055 1053 1056 - $xactions = $this->filterTransactions($object, $xactions); 1054 + if ($errors) { 1055 + throw new PhabricatorApplicationTransactionValidationException( 1056 + $errors); 1057 + } 1057 1058 1058 - if (!$is_preview) { 1059 - $this->hasRequiredMFA = true; 1060 - if ($this->getShouldRequireMFA()) { 1061 - $this->requireMFA($object, $xactions); 1059 + if ($this->raiseWarnings) { 1060 + $warnings = array(); 1061 + foreach ($xactions as $xaction) { 1062 + if ($this->hasWarnings($object, $xaction)) { 1063 + $warnings[] = $xaction; 1064 + } 1065 + } 1066 + if ($warnings) { 1067 + throw new PhabricatorApplicationTransactionWarningException( 1068 + $warnings); 1069 + } 1070 + } 1062 1071 } 1063 1072 1064 - if ($object->getID()) { 1065 - $this->buildOldRecipientLists($object, $xactions); 1073 + foreach ($xactions as $xaction) { 1074 + $this->adjustTransactionValues($object, $xaction); 1075 + } 1066 1076 1067 - $object->openTransaction(); 1068 - $transaction_open = true; 1077 + // Now that we've merged and combined transactions, check for required 1078 + // capabilities. Note that we're doing this before filtering 1079 + // transactions: if you try to apply an edit which you do not have 1080 + // permission to apply, we want to give you a permissions error even 1081 + // if the edit would have no effect. 1082 + $this->applyCapabilityChecks($object, $xactions); 1069 1083 1070 - $object->beginReadLocking(); 1071 - $read_locking = true; 1084 + $xactions = $this->filterTransactions($object, $xactions); 1072 1085 1073 - $object->reload(); 1074 - } 1086 + if (!$is_preview) { 1087 + $this->hasRequiredMFA = true; 1088 + if ($this->getShouldRequireMFA()) { 1089 + $this->requireMFA($object, $xactions); 1090 + } 1075 1091 1076 - if ($this->shouldApplyInitialEffects($object, $xactions)) { 1077 - if (!$transaction_open) { 1078 - $object->openTransaction(); 1079 - $transaction_open = true; 1092 + if ($this->shouldApplyInitialEffects($object, $xactions)) { 1093 + if (!$transaction_open) { 1094 + $object->openTransaction(); 1095 + $transaction_open = true; 1096 + } 1080 1097 } 1081 1098 } 1082 - } 1083 1099 1084 - try { 1085 1100 if ($this->shouldApplyInitialEffects($object, $xactions)) { 1086 1101 $this->applyInitialEffects($object, $xactions); 1087 1102 }