From 3d70db9eb5d052a8eff5b7c8f5e85b19e12a72cd Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Wed, 14 Jun 2017 12:10:39 -0700
Subject: [PATCH] Queue a worker task to send mail only after committing the
mail transaction
Summary:
Fixes T12844. This code is misleading: the daemon insert is happening on a different connection, and is not inside the transaction on the Mail connection.
What actually happens is this:
- (Connection A) `BEGIN`
- (Connection A) `INSERT INTO mail ...`
- (Connection B) `INSERT INTO worker ...` <-- This is a different connection, and it is NOT in a transaction!
- There's a race window here: the worker row is globally visible but the mail row is still isolated inside the transaction.
- (Connection A) `COMMIT`
- Now we're clear: the mail row is globally visible.
Change this code to reflect what's actually happening.
This means that if the worker row insert fails for some reason, we'll now throw with a mail row written to the database. But this is fine: it doesn't send on its own (so it can't cause mail loops or anything) and it can be re-queued with `bin/mail resend` if necessary without too much trouble.
Test Plan: See T12844 for particulars. Made some comments on tasks, saw the daemons send mail.
Reviewers: chad, amckinley, jmeador
Reviewed By: jmeador
Maniphest Tasks: T12844
Differential Revision: https://secure.phabricator.com/D18124
---
.../metamta/storage/PhabricatorMetaMTAMail.php | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/src/applications/metamta/storage/PhabricatorMetaMTAMail.php b/src/applications/metamta/storage/PhabricatorMetaMTAMail.php
index f2ed8c7721..20b1482036 100644
--- a/src/applications/metamta/storage/PhabricatorMetaMTAMail.php
+++ b/src/applications/metamta/storage/PhabricatorMetaMTAMail.php
@@ -372,16 +372,16 @@ final class PhabricatorMetaMTAMail
}
$editor->save();
- // Queue a task to send this mail.
- $mailer_task = PhabricatorWorker::scheduleTask(
- 'PhabricatorMetaMTAWorker',
- $this->getID(),
- array(
- 'priority' => PhabricatorWorker::PRIORITY_ALERTS,
- ));
-
$this->saveTransaction();
+ // Queue a task to send this mail.
+ $mailer_task = PhabricatorWorker::scheduleTask(
+ 'PhabricatorMetaMTAWorker',
+ $this->getID(),
+ array(
+ 'priority' => PhabricatorWorker::PRIORITY_ALERTS,
+ ));
+
return $result;
}