revi-archive/phorge-phorge
1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-11-26 16:52:41 +01:00
Code Issues Releases Wiki Activity

Fix a minor/harmless race with feed publishers in certain draft states

Browse source 
Summary:
Depends on D18851. Ref T13035. After D18819, revision creation transactions may be split into two groups (if prototypes are enabled).

This split means we have two workers. The first worker doesn't publish feed stories or mail; the second one does.

Currently, both workers call `shouldPublishFeedStory()` before they queue, and then again after the daemons pull them out of the queue. However, the answer to this question can change.

Specifically, this happens:

  - `arc` creates a revision.
  - The first transaction group applies, creating the revision as a draft, and returns `false` from `shouldPublishFeedStory()`, and does not generate related PHIDs. It queues a daemon to send mail, expecting it not to publish a feed story.
  - The second transaction group applies, promoting the revision to "needs review". Since the revision has promoted, `shouldPublishFeedStory()` now returns true. This editor generates related PHIDs and queues a daemon task, expecting it to send mail / publish feed.
  - A few milliseconds pass.
  - The first job gets pulled out of the daemon queue and worked on. It does not have any feed metadata because the object wasn't publishable when the job was queued -- but `shouldPublishFeedStory()` now returns true, so it tries to publish a story without any metadata available. Slightly bad stuff happens (see below).
  - The second job gets pulled out of the daemon queue and worked on. This one has metadata and works fine.

The "slightly bad stuff" is that we publish an empty feed story with no references to any objects, then try to push it to hooks and other listeners. Since it doesn't have any references, it fails to load during the "push to external listeners" phase.

This is harmless but clutters the log and doesn't help anything.

Instead, cache the state of "are we publishing a feed story for this object?" when we queue the worker so it can't race.

Test Plan:
  - Enabled prototypes.
  - Disabled all Herald triggers for Harbormaster build plans.
  - Ran `bin/phd debug task` in one window.
  - Created a revision in a separate window.
  - Before patch: saw "unable to load feed story" errors in the daemon log.
  - After patch: no more "unable to load feed story" errors.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13035

Differential Revision: https://secure.phabricator.com/D18852
This commit is contained in:
epriestley 2018-01-03 13:47:56 -08:00
parent de6c68b91e
commit 129e3a1208
2 changed files with 7 additions and 3 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

4
src/applications/feed/worker/FeedPushWorker.php
View file

@ -13,7 +13,9 @@ abstract class FeedPushWorker extends PhabricatorWorker {
if (!$story) {
throw new PhabricatorWorkerPermanentFailureException(
pht('Feed story does not exist.'));
pht(
'Feed story (with key "%s") does not exist or could not be loaded.',
$key));
}
return $story;

6
src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php
View file

@ -68,6 +68,7 @@ abstract class PhabricatorApplicationTransactionEditor
private $mailCCPHIDs = array();
private $feedNotifyPHIDs = array();
private $feedRelatedPHIDs = array();
private $feedShouldPublish = false;
private $modularTypes;
private $transactionQueue = array();
@ -1159,6 +1160,7 @@ abstract class PhabricatorApplicationTransactionEditor
}
if ($this->shouldPublishFeedStory($object, $xactions)) {
$this->feedShouldPublish = true;
$this->feedRelatedPHIDs = $this->getFeedRelatedPHIDs($object, $xactions);
$this->feedNotifyPHIDs = $this->getFeedNotifyPHIDs($object, $xactions);
}
@ -1216,8 +1218,7 @@ abstract class PhabricatorApplicationTransactionEditor
));
}
if ($this->shouldPublishFeedStory($object, $xactions)) {
if ($this->feedShouldPublish) {
$mailed = array();
foreach ($messages as $mail) {
foreach ($mail->buildRecipientList() as $phid) {
@ -3512,6 +3513,7 @@ abstract class PhabricatorApplicationTransactionEditor
'mailCCPHIDs',
'feedNotifyPHIDs',
'feedRelatedPHIDs',
'feedShouldPublish',
);
}