1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2025-03-20 00:00:10 +01:00

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
This commit is contained in:
epriestley 2017-06-14 12:10:39 -07:00
parent e1850b3c4e
commit 3d70db9eb5

View file

@ -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;
}