Page MenuHomePhabricator
Files F15352270

D19059.id45692.diff
No OneTemporary
Actions

D19059.id45692.diff
View Options

diff --git a/src/applications/auth/engine/PhabricatorAuthPasswordEngine.php b/src/applications/auth/engine/PhabricatorAuthPasswordEngine.php
--- a/src/applications/auth/engine/PhabricatorAuthPasswordEngine.php
+++ b/src/applications/auth/engine/PhabricatorAuthPasswordEngine.php
@@ -300,6 +300,10 @@
$password->upgradePasswordHasher($envelope, $this->getObject());
$new_hasher = $password->getHasher();
+ // NOTE: We must save the change before applying transactions because
+ // the editor will reload the object to obtain a read lock.
+ $password->save();
+
$xactions = array();
$xactions[] = $password->getApplicationTransactionTemplate()
diff --git a/src/applications/conpherence/editor/ConpherenceEditor.php b/src/applications/conpherence/editor/ConpherenceEditor.php
--- a/src/applications/conpherence/editor/ConpherenceEditor.php
+++ b/src/applications/conpherence/editor/ConpherenceEditor.php
@@ -99,24 +99,6 @@
return pht('%s created this room.', $author);
}
- /**
- * We really only need a read lock if we have a comment. In that case, we
- * must update the messagesCount field on the conpherence and
- * seenMessagesCount(s) for the participant(s).
- */
- protected function shouldReadLock(
- PhabricatorLiskDAO $object,
- PhabricatorApplicationTransaction $xaction) {
-
- $lock = false;
- switch ($xaction->getTransactionType()) {
- case PhabricatorTransactions::TYPE_COMMENT:
- $lock = true;
- break;
- }
-
- return $lock;
- }
protected function applyBuiltinInternalTransaction(
PhabricatorLiskDAO $object,
diff --git a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php
--- a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php
+++ b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php
@@ -924,23 +924,13 @@
if ($object->getID()) {
$this->buildOldRecipientLists($object, $xactions);
- foreach ($xactions as $xaction) {
+ $object->openTransaction();
+ $transaction_open = true;
- // If any of the transactions require a read lock, hold one and
- // reload the object. We need to do this fairly early so that the
- // call to `adjustTransactionValues()` (which populates old values)
- // is based on the synchronized state of the object, which may differ
- // from the state when it was originally loaded.
-
- if ($this->shouldReadLock($object, $xaction)) {
- $object->openTransaction();
- $object->beginReadLocking();
- $transaction_open = true;
- $read_locking = true;
- $object->reload();
- break;
- }
- }
+ $object->beginReadLocking();
+ $read_locking = true;
+
+ $object->reload();
}
if ($this->shouldApplyInitialEffects($object, $xactions)) {
@@ -951,66 +941,57 @@
}
}
- if ($this->shouldApplyInitialEffects($object, $xactions)) {
- $this->applyInitialEffects($object, $xactions);
- }
+ try {
+ if ($this->shouldApplyInitialEffects($object, $xactions)) {
+ $this->applyInitialEffects($object, $xactions);
+ }
- foreach ($xactions as $xaction) {
- $this->adjustTransactionValues($object, $xaction);
- }
+ foreach ($xactions as $xaction) {
+ $this->adjustTransactionValues($object, $xaction);
+ }
- try {
$xactions = $this->filterTransactions($object, $xactions);
- } catch (Exception $ex) {
- if ($read_locking) {
- $object->endReadLocking();
- }
- if ($transaction_open) {
- $object->killTransaction();
+
+ // TODO: Once everything is on EditEngine, just use getIsNewObject() to
+ // figure this out instead.
+ $mark_as_create = false;
+ $create_type = PhabricatorTransactions::TYPE_CREATE;
+ foreach ($xactions as $xaction) {
+ if ($xaction->getTransactionType() == $create_type) {
+ $mark_as_create = true;
+ }
}
- throw $ex;
- }
- // TODO: Once everything is on EditEngine, just use getIsNewObject() to
- // figure this out instead.
- $mark_as_create = false;
- $create_type = PhabricatorTransactions::TYPE_CREATE;
- foreach ($xactions as $xaction) {
- if ($xaction->getTransactionType() == $create_type) {
- $mark_as_create = true;
+ if ($mark_as_create) {
+ foreach ($xactions as $xaction) {
+ $xaction->setIsCreateTransaction(true);
+ }
}
- }
- if ($mark_as_create) {
+ // Now that we've merged, filtered, and combined transactions, check for
+ // required capabilities.
foreach ($xactions as $xaction) {
- $xaction->setIsCreateTransaction(true);
+ $this->requireCapabilities($object, $xaction);
}
- }
- // Now that we've merged, filtered, and combined transactions, check for
- // required capabilities.
- foreach ($xactions as $xaction) {
- $this->requireCapabilities($object, $xaction);
- }
+ $xactions = $this->sortTransactions($xactions);
+ $file_phids = $this->extractFilePHIDs($object, $xactions);
- $xactions = $this->sortTransactions($xactions);
- $file_phids = $this->extractFilePHIDs($object, $xactions);
-
- if ($is_preview) {
- $this->loadHandles($xactions);
- return $xactions;
- }
+ if ($is_preview) {
+ $this->loadHandles($xactions);
+ return $xactions;
+ }
- $comment_editor = id(new PhabricatorApplicationTransactionCommentEditor())
- ->setActor($actor)
- ->setActingAsPHID($this->getActingAsPHID())
- ->setContentSource($this->getContentSource());
+ $comment_editor = id(new PhabricatorApplicationTransactionCommentEditor())
+ ->setActor($actor)
+ ->setActingAsPHID($this->getActingAsPHID())
+ ->setContentSource($this->getContentSource());
- if (!$transaction_open) {
- $object->openTransaction();
- }
+ if (!$transaction_open) {
+ $object->openTransaction();
+ $transaction_open = true;
+ }
- try {
foreach ($xactions as $xaction) {
$this->applyInternalEffects($object, $xaction);
}
@@ -1070,14 +1051,27 @@
}
$xactions = $this->applyFinalEffects($object, $xactions);
+
if ($read_locking) {
$object->endReadLocking();
$read_locking = false;
}
- $object->saveTransaction();
+ if ($transaction_open) {
+ $object->saveTransaction();
+ $transaction_open = false;
+ }
} catch (Exception $ex) {
- $object->killTransaction();
+ if ($read_locking) {
+ $object->endReadLocking();
+ $read_locking = false;
+ }
+
+ if ($transaction_open) {
+ $object->killTransaction();
+ $transaction_open = false;
+ }
+
throw $ex;
}
@@ -1319,46 +1313,6 @@
return $xactions;
}
-
- /**
- * Determine if the editor should hold a read lock on the object while
- * applying a transaction.
- *
- * If the editor does not hold a lock, two editors may read an object at the
- * same time, then apply their changes without any synchronization. For most
- * transactions, this does not matter much. However, it is important for some
- * transactions. For example, if an object has a transaction count on it, both
- * editors may read the object with `count = 23`, then independently update it
- * and save the object with `count = 24` twice. This will produce the wrong
- * state: the object really has 25 transactions, but the count is only 24.
- *
- * Generally, transactions fall into one of four buckets:
- *
- * - Append operations: Actions like adding a comment to an object purely
- * add information to its state, and do not depend on the current object
- * state in any way. These transactions never need to hold locks.
- * - Overwrite operations: Actions like changing the title or description
- * of an object replace the current value with a new value, so the end
- * state is consistent without a lock. We currently do not lock these
- * transactions, although we may in the future.
- * - Edge operations: Edge and subscription operations have internal
- * synchronization which limits the damage race conditions can cause.
- * We do not currently lock these transactions, although we may in the
- * future.
- * - Update operations: Actions like incrementing a count on an object.
- * These operations generally should use locks, unless it is not
- * important that the state remain consistent in the presence of races.
- *
- * @param PhabricatorLiskDAO Object being updated.
- * @param PhabricatorApplicationTransaction Transaction being applied.
- * @return bool True to synchronize the edit with a lock.
- */
- protected function shouldReadLock(
- PhabricatorLiskDAO $object,
- PhabricatorApplicationTransaction $xaction) {
- return false;
- }
-
private function loadHandles(array $xactions) {
$phids = array();
foreach ($xactions as $key => $xaction) {

File Metadata

Mime Type
text/plain
Expires
Tue, Mar 11, 4:18 PM (1 w, 3 d ago)
Storage Engine
blob
Storage Format
Encrypted (AES-256-CBC)
Storage Handle
7383285
Default Alt Text
D19059.id45692.diff (9 KB)

Event Timeline

Phabricator · Built by Phacility