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.

Move the "container updated" message for Buildables that build Diffs outside of the transaction

Summary:
Ref T13216. See PHI970. Ref T13054. See some discussion in T13216.

When a Harbormaster Buildable object is first created for a Diff, it has no `containerPHID` since the revision has not yet been created.

We later (after creating a revision) send the Buildable a message telling it that we've added a container and it should re-link the container object.

Currently, we send this message in `applyExternalEffects()`, which runs inside the Differential transaction. If Harbormaster races quickly enough, it can read the `Diff` object before the transaction commits, and not see the container update.

Add a `didCommitTransaction()` callback after the transactions commit, then move the message code there instead.

Test Plan:
- See T13216 for substantial evidence that this change is on the right track.
- Before change: added `sleep(15)`, reproduced the issue reliably.
- After change: unable to reproduce issue even with `sleep(15)` (the `containerPHID` always populates correctly).

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13216, T13054

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

epriestley b2e91d22 44c32839

+47 -12
+7 -1
src/applications/differential/xaction/DifferentialRevisionUpdateTransaction.php
··· 57 57 // Harbormaster. See discussion in T8650. 58 58 $diff->setRevisionID($object->getID()); 59 59 $diff->save(); 60 + } 61 + 62 + public function didCommitTransaction($object, $value) { 63 + $editor = $this->getEditor(); 64 + $diff = $editor->requireDiff($value); 65 + $omnipotent = PhabricatorUser::getOmnipotentUser(); 60 66 61 67 // If there are any outstanding buildables for this diff, tell 62 68 // Harbormaster that their containers need to be updated. This is ··· 64 70 // and unit results. 65 71 66 72 $buildables = id(new HarbormasterBuildableQuery()) 67 - ->setViewer(PhabricatorUser::getOmnipotentUser()) 73 + ->setViewer($omnipotent) 68 74 ->withManualBuildables(false) 69 75 ->withBuildablePHIDs(array($diff->getPHID())) 70 76 ->execute();
+21
src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php
··· 869 869 return $xactions; 870 870 } 871 871 872 + final protected function didCommitTransactions( 873 + PhabricatorLiskDAO $object, 874 + array $xactions) { 875 + 876 + foreach ($xactions as $xaction) { 877 + $type = $xaction->getTransactionType(); 878 + 879 + $xtype = $this->getModularTransactionType($type); 880 + if (!$xtype) { 881 + continue; 882 + } 883 + 884 + $xtype = clone $xtype; 885 + $xtype->setStorage($xaction); 886 + $xtype->didCommitTransaction($object, $xaction->getNewValue()); 887 + } 888 + } 889 + 872 890 public function setContentSource(PhabricatorContentSource $content_source) { 873 891 $this->contentSource = $content_source; 874 892 return $this; ··· 1106 1124 $object->saveTransaction(); 1107 1125 $transaction_open = false; 1108 1126 } 1127 + 1128 + $this->didCommitTransactions($object, $xactions); 1129 + 1109 1130 } catch (Exception $ex) { 1110 1131 if ($read_locking) { 1111 1132 $object->endReadLocking();
+4
src/applications/transactions/storage/PhabricatorModularTransactionType.php
··· 35 35 return; 36 36 } 37 37 38 + public function didCommitTransaction($object, $value) { 39 + return; 40 + } 41 + 38 42 public function getTransactionHasEffect($object, $old, $new) { 39 43 return ($old !== $new); 40 44 }
+8 -8
src/infrastructure/daemon/workers/query/PhabricatorWorkerTaskQuery.php
··· 103 103 return $this->formatWhereClause($conn, $where); 104 104 } 105 105 106 - protected function buildOrderClause(AphrontDatabaseConnection $conn_r) { 106 + protected function buildOrderClause(AphrontDatabaseConnection $conn) { 107 107 // NOTE: The garbage collector executes this query with a date constraint, 108 108 // and the query is inefficient if we don't use the same key for ordering. 109 109 // See T9808 for discussion. 110 110 111 111 if ($this->dateCreatedBefore) { 112 - return qsprintf($conn_r, 'ORDER BY dateCreated DESC, id DESC'); 112 + return qsprintf($conn, 'ORDER BY dateCreated DESC, id DESC'); 113 113 } else if ($this->dateModifiedSince) { 114 - return qsprintf($conn_r, 'ORDER BY dateModified DESC, id DESC'); 114 + return qsprintf($conn, 'ORDER BY dateModified DESC, id DESC'); 115 115 } else { 116 - return qsprintf($conn_r, 'ORDER BY id DESC'); 116 + return qsprintf($conn, 'ORDER BY id DESC'); 117 117 } 118 118 } 119 119 120 - protected function buildLimitClause(AphrontDatabaseConnection $conn_r) { 121 - $clause = ''; 120 + protected function buildLimitClause(AphrontDatabaseConnection $conn) { 122 121 if ($this->limit) { 123 - $clause = qsprintf($conn_r, 'LIMIT %d', $this->limit); 122 + return qsprintf($conn, 'LIMIT %d', $this->limit); 123 + } else { 124 + return qsprintf($conn, ''); 124 125 } 125 - return $clause; 126 126 } 127 127 128 128 }
+7 -3
src/infrastructure/daemon/workers/query/PhabricatorWorkerTriggerQuery.php
··· 145 145 return $triggers; 146 146 } 147 147 148 - protected function buildJoinClause(AphrontDatabaseConnection $conn_r) { 148 + protected function buildJoinClause(AphrontDatabaseConnection $conn) { 149 149 $joins = array(); 150 150 151 151 if (($this->nextEpochMin !== null) || 152 152 ($this->nextEpochMax !== null) || 153 153 ($this->order == self::ORDER_EXECUTION)) { 154 154 $joins[] = qsprintf( 155 - $conn_r, 155 + $conn, 156 156 'JOIN %T e ON e.triggerID = t.id', 157 157 id(new PhabricatorWorkerTriggerEvent())->getTableName()); 158 158 } 159 159 160 - return implode(' ', $joins); 160 + if ($joins) { 161 + return qsprintf($conn, '%LJ', $joins); 162 + } else { 163 + return qsprintf($conn, ''); 164 + } 161 165 } 162 166 163 167 protected function buildWhereClause(AphrontDatabaseConnection $conn) {