diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -3178,6 +3178,7 @@ 'PhabricatorMacroTransactionQuery' => 'applications/macro/query/PhabricatorMacroTransactionQuery.php', 'PhabricatorMacroTransactionType' => 'applications/macro/xaction/PhabricatorMacroTransactionType.php', 'PhabricatorMacroViewController' => 'applications/macro/controller/PhabricatorMacroViewController.php', + 'PhabricatorMailConfigTestCase' => 'applications/metamta/storage/__tests__/PhabricatorMailConfigTestCase.php', 'PhabricatorMailEmailHeraldField' => 'applications/metamta/herald/PhabricatorMailEmailHeraldField.php', 'PhabricatorMailEmailHeraldFieldGroup' => 'applications/metamta/herald/PhabricatorMailEmailHeraldFieldGroup.php', 'PhabricatorMailEmailSubjectHeraldField' => 'applications/metamta/herald/PhabricatorMailEmailSubjectHeraldField.php', @@ -8680,6 +8681,7 @@ 'PhabricatorMacroTransactionQuery' => 'PhabricatorApplicationTransactionQuery', 'PhabricatorMacroTransactionType' => 'PhabricatorModularTransactionType', 'PhabricatorMacroViewController' => 'PhabricatorMacroController', + 'PhabricatorMailConfigTestCase' => 'PhabricatorTestCase', 'PhabricatorMailEmailHeraldField' => 'HeraldField', 'PhabricatorMailEmailHeraldFieldGroup' => 'HeraldFieldGroup', 'PhabricatorMailEmailSubjectHeraldField' => 'PhabricatorMailEmailHeraldField', diff --git a/src/applications/metamta/storage/PhabricatorMetaMTAMail.php b/src/applications/metamta/storage/PhabricatorMetaMTAMail.php --- a/src/applications/metamta/storage/PhabricatorMetaMTAMail.php +++ b/src/applications/metamta/storage/PhabricatorMetaMTAMail.php @@ -513,12 +513,14 @@ $defaults = $mailer->newDefaultOptions(); $options = idx($spec, 'options', array()) + $defaults; $mailer->setOptions($options); + + $mailers[] = $mailer; } } $sorted = array(); $groups = mgroup($mailers, 'getPriority'); - ksort($groups); + krsort($groups); foreach ($groups as $group) { // Reorder services within the same priority group randomly. shuffle($group); diff --git a/src/applications/metamta/storage/__tests__/PhabricatorMailConfigTestCase.php b/src/applications/metamta/storage/__tests__/PhabricatorMailConfigTestCase.php new file mode 100644 --- /dev/null +++ b/src/applications/metamta/storage/__tests__/PhabricatorMailConfigTestCase.php @@ -0,0 +1,131 @@ +newMailersWithConfig( + array( + array( + 'key' => 'A', + 'type' => 'test', + ), + array( + 'key' => 'B', + 'type' => 'test', + ), + )); + + $this->assertEqual( + array('A', 'B'), + mpull($mailers, 'getKey')); + + $mailers = $this->newMailersWithConfig( + array( + array( + 'key' => 'A', + 'priority' => 1, + 'type' => 'test', + ), + array( + 'key' => 'B', + 'priority' => 2, + 'type' => 'test', + ), + )); + + $this->assertEqual( + array('B', 'A'), + mpull($mailers, 'getKey')); + + $mailers = $this->newMailersWithConfig( + array( + array( + 'key' => 'A1', + 'priority' => 300, + 'type' => 'test', + ), + array( + 'key' => 'A2', + 'priority' => 300, + 'type' => 'test', + ), + array( + 'key' => 'B', + 'type' => 'test', + ), + array( + 'key' => 'C', + 'priority' => 400, + 'type' => 'test', + ), + array( + 'key' => 'D', + 'type' => 'test', + ), + )); + + // The "A" servers should be shuffled randomly, so either outcome is + // acceptable. + $option_1 = array('C', 'A1', 'A2', 'B', 'D'); + $option_2 = array('C', 'A2', 'A1', 'B', 'D'); + $actual = mpull($mailers, 'getKey'); + + $this->assertTrue(($actual === $option_1) || ($actual === $option_2)); + + // Make sure that when we're load balancing we actually send traffic to + // both servers reasonably often. + $saw_a1 = false; + $saw_a2 = false; + $attempts = 0; + while (true) { + $mailers = $this->newMailersWithConfig( + array( + array( + 'key' => 'A1', + 'priority' => 300, + 'type' => 'test', + ), + array( + 'key' => 'A2', + 'priority' => 300, + 'type' => 'test', + ), + )); + + $first_key = head($mailers)->getKey(); + + if ($first_key == 'A1') { + $saw_a1 = true; + } + + if ($first_key == 'A2') { + $saw_a2 = true; + } + + if ($saw_a1 && $saw_a2) { + break; + } + + if ($attempts++ > 1024) { + throw new Exception( + pht( + 'Load balancing between two mail servers did not select both '. + 'servers after an absurd number of attempts.')); + } + } + + $this->assertTrue($saw_a1 && $saw_a2); + } + + private function newMailersWithConfig(array $config) { + $env = PhabricatorEnv::beginScopedEnv(); + $env->overrideEnvConfig('cluster.mailers', $config); + + $mailers = PhabricatorMetaMTAMail::newMailers(); + + unset($env); + return $mailers; + } + +}