From 4b0ef353e48d95d678acfd96232aa87cbdfe34d1 Mon Sep 17 00:00:00 2001 From: epriestley Date: Sat, 1 Feb 2014 14:35:42 -0800 Subject: [PATCH] Remove retry/failure mechanisms from MetaMTA Summary: Fixes T4202. We have old code in MetaMTA which implements gradual backoff and maximum retries. However, we have more general code in the task queue which does this, too. We can just use the more general stuff in the task queue; it obsoletes the specific stuff in MetaMTA, which is more complex and ran into some kind of issue in T4202. Remove `retryCount`, `nextRetry` (obsoleted by task queue retry mechanisms) and "simulated failures" (no longer in use). Generally, modern infrastructure has replaced these mechanisms with more general ones. Test Plan: - Sent mail. - Observed unsendable mail failing in reasonable ways in the queue. Reviewers: btrahan Reviewed By: btrahan CC: aran Maniphest Tasks: T4202 Differential Revision: https://secure.phabricator.com/D8115 --- .../sql/autopatches/20140130.mail.1.retry.sql | 2 + .../sql/autopatches/20140130.mail.2.next.sql | 2 + .../metamta/PhabricatorMetaMTAWorker.php | 35 ++++++------- ...habricatorMailManagementResendWorkflow.php | 3 -- ...atorMailManagementShowOutboundWorkflow.php | 2 - .../storage/PhabricatorMetaMTAMail.php | 50 ++++--------------- 6 files changed, 29 insertions(+), 65 deletions(-) create mode 100644 resources/sql/autopatches/20140130.mail.1.retry.sql create mode 100644 resources/sql/autopatches/20140130.mail.2.next.sql diff --git a/resources/sql/autopatches/20140130.mail.1.retry.sql b/resources/sql/autopatches/20140130.mail.1.retry.sql new file mode 100644 index 0000000000..42ff5afab2 --- /dev/null +++ b/resources/sql/autopatches/20140130.mail.1.retry.sql @@ -0,0 +1,2 @@ +ALTER TABLE {$NAMESPACE}_metamta.metamta_mail + DROP COLUMN retryCount; diff --git a/resources/sql/autopatches/20140130.mail.2.next.sql b/resources/sql/autopatches/20140130.mail.2.next.sql new file mode 100644 index 0000000000..78e5693cf1 --- /dev/null +++ b/resources/sql/autopatches/20140130.mail.2.next.sql @@ -0,0 +1,2 @@ +ALTER TABLE {$NAMESPACE}_metamta.metamta_mail + DROP COLUMN nextRetry; diff --git a/src/applications/metamta/PhabricatorMetaMTAWorker.php b/src/applications/metamta/PhabricatorMetaMTAWorker.php index 7d218e5fd8..d9defcd8df 100644 --- a/src/applications/metamta/PhabricatorMetaMTAWorker.php +++ b/src/applications/metamta/PhabricatorMetaMTAWorker.php @@ -3,42 +3,39 @@ final class PhabricatorMetaMTAWorker extends PhabricatorWorker { - private $message; + public function getMaximumRetryCount() { + return 250; + } public function getWaitBeforeRetry(PhabricatorWorkerTask $task) { - $message = $this->loadMessage(); - if (!$message) { - return null; - } - - $wait = max($message->getNextRetry() - time(), 0) + 15; - return $wait; + return ($task->getFailureCount() * 15); } public function doWork() { $message = $this->loadMessage(); - if (!$message - || $message->getStatus() != PhabricatorMetaMTAMail::STATUS_QUEUE) { + if (!$message) { + throw new PhabricatorWorkerPermanentFailureException( + pht('Unable to load message!')); + } + + if ($message->getStatus() != PhabricatorMetaMTAMail::STATUS_QUEUE) { return; } + $id = $message->getID(); $message->sendNow(); + // task failed if the message is still queued // (instead of sent, void, or failed) if ($message->getStatus() == PhabricatorMetaMTAMail::STATUS_QUEUE) { - throw new Exception('Failed to send message'); + throw new Exception( + pht('Failed to send message.')); } } private function loadMessage() { - if (!$this->message) { - $message_id = $this->getTaskData(); - $this->message = id(new PhabricatorMetaMTAMail())->load($message_id); - if (!$this->message) { - return null; - } - } - return $this->message; + $message_id = $this->getTaskData(); + return id(new PhabricatorMetaMTAMail())->load($message_id); } public function renderForDisplay(PhabricatorUser $viewer) { diff --git a/src/applications/metamta/management/PhabricatorMailManagementResendWorkflow.php b/src/applications/metamta/management/PhabricatorMailManagementResendWorkflow.php index c4ba433332..1a4ef6ba70 100644 --- a/src/applications/metamta/management/PhabricatorMailManagementResendWorkflow.php +++ b/src/applications/metamta/management/PhabricatorMailManagementResendWorkflow.php @@ -54,9 +54,6 @@ final class PhabricatorMailManagementResendWorkflow } $message->setStatus(PhabricatorMetaMTAMail::STATUS_QUEUE); - $message->setRetryCount(0); - $message->setNextRetry(time()); - $message->save(); $mailer_task = PhabricatorWorker::scheduleTask( diff --git a/src/applications/metamta/management/PhabricatorMailManagementShowOutboundWorkflow.php b/src/applications/metamta/management/PhabricatorMailManagementShowOutboundWorkflow.php index d416137610..31989fa261 100644 --- a/src/applications/metamta/management/PhabricatorMailManagementShowOutboundWorkflow.php +++ b/src/applications/metamta/management/PhabricatorMailManagementShowOutboundWorkflow.php @@ -50,8 +50,6 @@ final class PhabricatorMailManagementShowOutboundWorkflow $info[] = pht('PROPERTIES'); $info[] = pht('ID: %d', $message->getID()); $info[] = pht('Status: %s', $message->getStatus()); - $info[] = pht('Retry Count: %s', $message->getRetryCount()); - $info[] = pht('Next Retry: %s', $message->getNextRetry()); $info[] = pht('Related PHID: %s', $message->getRelatedPHID()); $info[] = pht('Message: %s', $message->getMessage()); diff --git a/src/applications/metamta/storage/PhabricatorMetaMTAMail.php b/src/applications/metamta/storage/PhabricatorMetaMTAMail.php index ec01faf9a2..c0ce3fe061 100644 --- a/src/applications/metamta/storage/PhabricatorMetaMTAMail.php +++ b/src/applications/metamta/storage/PhabricatorMetaMTAMail.php @@ -1,8 +1,6 @@ status = self::STATUS_QUEUE; - $this->retryCount = 0; - $this->nextRetry = time(); $this->parameters = array(); parent::__construct(); @@ -228,15 +221,6 @@ final class PhabricatorMetaMTAMail extends PhabricatorMetaMTADAO { return $this; } - public function getSimulatedFailureCount() { - return nonempty($this->getParam('simulated-failures'), 0); - } - - public function setSimulatedFailureCount($count) { - $this->setParam('simulated-failures', $count); - return $this; - } - public function getWorkerTaskID() { return $this->getParam('worker-task'); } @@ -339,10 +323,6 @@ final class PhabricatorMetaMTAMail extends PhabricatorMetaMTADAO { if ($this->getStatus() != self::STATUS_QUEUE) { throw new Exception("Trying to send an already-sent mail!"); } - - if (time() < $this->getNextRetry()) { - throw new Exception("Trying to send an email before next retry!"); - } } try { @@ -611,32 +591,20 @@ final class PhabricatorMetaMTAMail extends PhabricatorMetaMTADAO { return $this->save(); } - if ($this->getRetryCount() < $this->getSimulatedFailureCount()) { + try { + $ok = $mailer->send(); + $error = null; + } catch (PhabricatorMetaMTAPermanentFailureException $ex) { + $this->setStatus(self::STATUS_FAIL); + $this->setMessage($ex->getMessage()); + return $this->save(); + } catch (Exception $ex) { $ok = false; - $error = 'Simulated failure.'; - } else { - try { - $ok = $mailer->send(); - $error = null; - } catch (PhabricatorMetaMTAPermanentFailureException $ex) { - $this->setStatus(self::STATUS_FAIL); - $this->setMessage($ex->getMessage()); - return $this->save(); - } catch (Exception $ex) { - $ok = false; - $error = $ex->getMessage()."\n".$ex->getTraceAsString(); - } + $error = $ex->getMessage()."\n".$ex->getTraceAsString(); } if (!$ok) { $this->setMessage($error); - if ($this->getRetryCount() > self::MAX_RETRIES) { - $this->setStatus(self::STATUS_FAIL); - } else { - $this->setRetryCount($this->getRetryCount() + 1); - $next_retry = time() + ($this->getRetryCount() * self::RETRY_DELAY); - $this->setNextRetry($next_retry); - } } else { $this->setStatus(self::STATUS_SENT); }