Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
feature #35525 [Mailer] Randomize the first transport used by the Rou…
…ndRobin transport (fabpot)

This PR was merged into the 5.1-dev branch.

Discussion
----------

[Mailer] Randomize the first transport used by the RoundRobin transport

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes <!-- please update src/**/CHANGELOG.md files -->
| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tickets       | Fix #33723 <!-- prefix each issue number with "Fix #", if any -->
| License       | MIT
| Doc PR        | symfony/symfony-docs#... <!-- required for new features -->

When not using Messenger, and so sending only one message, the RoundRobin class does not work as the first transport is always used. This PR randomizes the first transport used by the class to mitigate that problem.

Commits
-------

6ebe83c [Mailer] Randomize the first transport used by the RoundRobin transport
  • Loading branch information
fabpot committed Jan 31, 2020
2 parents 1bb485b + 6ebe83c commit 09bdaf5
Show file tree
Hide file tree
Showing 3 changed files with 39 additions and 7 deletions.
Expand Up @@ -46,6 +46,9 @@ public function testSendFirstWork()
$t2 = $this->createMock(TransportInterface::class);
$t2->expects($this->never())->method('send');
$t = new FailoverTransport([$t1, $t2]);
$p = new \ReflectionProperty(RoundRobinTransport::class, 'cursor');
$p->setAccessible(true);
$p->setValue($t, 0);
$t->send(new RawMessage(''));
$this->assertTransports($t, 1, []);
$t->send(new RawMessage(''));
Expand Down Expand Up @@ -74,6 +77,9 @@ public function testSendOneDead()
$t2 = $this->createMock(TransportInterface::class);
$t2->expects($this->exactly(3))->method('send');
$t = new FailoverTransport([$t1, $t2]);
$p = new \ReflectionProperty(RoundRobinTransport::class, 'cursor');
$p->setAccessible(true);
$p->setValue($t, 0);
$t->send(new RawMessage(''));
$this->assertTransports($t, 0, [$t1]);
$t->send(new RawMessage(''));
Expand All @@ -93,6 +99,9 @@ public function testSendOneDeadAndRecoveryWithinRetryPeriod()
$t2->expects($this->at(2))->method('send');
$t2->expects($this->at(3))->method('send')->will($this->throwException(new TransportException()));
$t = new FailoverTransport([$t1, $t2], 6);
$p = new \ReflectionProperty(RoundRobinTransport::class, 'cursor');
$p->setAccessible(true);
$p->setValue($t, 0);
$t->send(new RawMessage('')); // t1>fail - t2>sent
$this->assertTransports($t, 0, [$t1]);
sleep(4);
Expand Down Expand Up @@ -139,6 +148,9 @@ public function testSendOneDeadButRecover()
$t2->expects($this->at(1))->method('send');
$t2->expects($this->at(2))->method('send')->will($this->throwException(new TransportException()));
$t = new FailoverTransport([$t1, $t2], 1);
$p = new \ReflectionProperty(RoundRobinTransport::class, 'cursor');
$p->setAccessible(true);
$p->setValue($t, 0);
$t->send(new RawMessage(''));
sleep(1);
$t->send(new RawMessage(''));
Expand Down
Expand Up @@ -41,16 +41,16 @@ public function testToString()
public function testSendAlternate()
{
$t1 = $this->createMock(TransportInterface::class);
$t1->expects($this->exactly(2))->method('send');
$t1->expects($this->atLeast(1))->method('send');
$t2 = $this->createMock(TransportInterface::class);
$t2->expects($this->once())->method('send');
$t2->expects($this->atLeast(1))->method('send');
$t = new RoundRobinTransport([$t1, $t2]);
$t->send(new RawMessage(''));
$this->assertTransports($t, 1, []);
$cursor = $this->assertTransports($t, -1, []);
$t->send(new RawMessage(''));
$this->assertTransports($t, 0, []);
$cursor = $this->assertTransports($t, 0 === $cursor ? 1 : 0, []);
$t->send(new RawMessage(''));
$this->assertTransports($t, 1, []);
$this->assertTransports($t, 0 === $cursor ? 1 : 0, []);
}

public function testSendAllDead()
Expand All @@ -73,6 +73,9 @@ public function testSendOneDead()
$t2 = $this->createMock(TransportInterface::class);
$t2->expects($this->exactly(3))->method('send');
$t = new RoundRobinTransport([$t1, $t2]);
$p = new \ReflectionProperty($t, 'cursor');
$p->setAccessible(true);
$p->setValue($t, 0);
$t->send(new RawMessage(''));
$this->assertTransports($t, 0, [$t1]);
$t->send(new RawMessage(''));
Expand All @@ -88,6 +91,9 @@ public function testSendOneDeadAndRecoveryNotWithinRetryPeriod()
$t2 = $this->createMock(TransportInterface::class);
$t2->expects($this->once())->method('send')->will($this->throwException(new TransportException()));
$t = new RoundRobinTransport([$t1, $t2], 60);
$p = new \ReflectionProperty($t, 'cursor');
$p->setAccessible(true);
$p->setValue($t, 0);
$t->send(new RawMessage(''));
$this->assertTransports($t, 1, []);
$t->send(new RawMessage(''));
Expand All @@ -106,6 +112,9 @@ public function testSendOneDeadAndRecoveryWithinRetryPeriod()
$t2->expects($this->at(0))->method('send')->will($this->throwException(new TransportException()));
$t2->expects($this->at(1))->method('send');
$t = new RoundRobinTransport([$t1, $t2], 3);
$p = new \ReflectionProperty($t, 'cursor');
$p->setAccessible(true);
$p->setValue($t, 0);
$t->send(new RawMessage(''));
$this->assertTransports($t, 1, []);
$t->send(new RawMessage(''));
Expand All @@ -121,10 +130,15 @@ private function assertTransports(RoundRobinTransport $transport, int $cursor, a
{
$p = new \ReflectionProperty($transport, 'cursor');
$p->setAccessible(true);
$this->assertSame($cursor, $p->getValue($transport));
if (-1 !== $cursor) {
$this->assertSame($cursor, $p->getValue($transport));
}
$cursor = $p->getValue($transport);

$p = new \ReflectionProperty($transport, 'deadTransports');
$p->setAccessible(true);
$this->assertSame($deadTransports, iterator_to_array($p->getValue($transport)));

return $cursor;
}
}
Expand Up @@ -27,7 +27,7 @@ class RoundRobinTransport implements TransportInterface
private $deadTransports;
private $transports = [];
private $retryPeriod;
private $cursor = 0;
private $cursor = -1;

/**
* @param TransportInterface[] $transports
Expand Down Expand Up @@ -66,6 +66,12 @@ public function __toString(): string
*/
protected function getNextTransport(): ?TransportInterface
{
if (-1 === $this->cursor) {
// the cursor initial value is randomized so that
// when are not in a daemon, we are still rotating the transports
$this->cursor = mt_rand(0, \count($this->transports) - 1);
}

$cursor = $this->cursor;
while (true) {
$transport = $this->transports[$cursor];
Expand Down

0 comments on commit 09bdaf5

Please sign in to comment.