Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
bug #31024 [Mailer] fixed roundrobin test one dead which should recov…
…er (scuben, fabpot)

This PR was merged into the 4.3-dev branch.

Discussion
----------

[Mailer] fixed roundrobin test one dead which should recover

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | WIP    <!-- please add some, will be required by reviewers -->
| License       | MIT
| Doc PR        | n/a

The Test `testSendOneDeadButRecover` is not checking the recovery part of its job. I fixed that by adding more `send` calls and added another test so that both recoveries (within retry period and not within retry period) are covered.

The `RoundRobinTransport` had a bug where the transport is dead but not yet in the `retryPeriod`. In that case the transport would not have been added back to the stack and thus got lost. Fixed that but that required an additional check if all transports are dead to prevent an infinite loop.

Commits
-------

5d4d4e7 fixed roundrobin dead transport which should recover
ccbb171 fixed roundrobin dead transport which should recover
  • Loading branch information
fabpot committed Apr 11, 2019
2 parents e0c4528 + 5d4d4e7 commit 751baaf
Show file tree
Hide file tree
Showing 3 changed files with 129 additions and 10 deletions.
Expand Up @@ -14,6 +14,7 @@
use PHPUnit\Framework\TestCase;
use Symfony\Component\Mailer\Exception\TransportException;
use Symfony\Component\Mailer\Transport\FailoverTransport;
use Symfony\Component\Mailer\Transport\RoundRobinTransport;
use Symfony\Component\Mailer\Transport\TransportInterface;
use Symfony\Component\Mime\RawMessage;

Expand All @@ -36,8 +37,11 @@ public function testSendFirstWork()
$t2->expects($this->never())->method('send');
$t = new FailoverTransport([$t1, $t2]);
$t->send(new RawMessage(''));
$this->assertTransports($t, 1, []);
$t->send(new RawMessage(''));
$this->assertTransports($t, 1, []);
$t->send(new RawMessage(''));
$this->assertTransports($t, 1, []);
}

public function testSendAllDead()
Expand All @@ -50,6 +54,7 @@ public function testSendAllDead()
$this->expectException(TransportException::class);
$this->expectExceptionMessage('All transports failed.');
$t->send(new RawMessage(''));
$this->assertTransports($t, 0, [$t1, $t2]);
}

public function testSendOneDead()
Expand All @@ -60,7 +65,57 @@ public function testSendOneDead()
$t2->expects($this->exactly(3))->method('send');
$t = new FailoverTransport([$t1, $t2]);
$t->send(new RawMessage(''));
$this->assertTransports($t, 0, [$t1]);
$t->send(new RawMessage(''));
$this->assertTransports($t, 0, [$t1]);
$t->send(new RawMessage(''));
$this->assertTransports($t, 0, [$t1]);
}

public function testSendOneDeadAndRecoveryWithinRetryPeriod()
{
$t1 = $this->createMock(TransportInterface::class);
$t1->expects($this->at(0))->method('send')->will($this->throwException(new TransportException()));
$t1->expects($this->at(1))->method('send');
$t2 = $this->createMock(TransportInterface::class);
$t2->expects($this->at(0))->method('send');
$t2->expects($this->at(1))->method('send');
$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);
$t->send(new RawMessage('')); // t1>fail - t2>sent
$this->assertTransports($t, 0, [$t1]);
sleep(4);
$t->send(new RawMessage('')); // t2>sent
$this->assertTransports($t, 0, [$t1]);
sleep(4);
$t->send(new RawMessage('')); // t2>sent
$this->assertTransports($t, 0, [$t1]);
sleep(4);
$t->send(new RawMessage('')); // t2>fail - t1>sent
$this->assertTransports($t, 1, [$t2]);
sleep(4);
$t->send(new RawMessage('')); // t1>sent
$this->assertTransports($t, 1, [$t2]);
}

public function testSendAllDeadWithinRetryPeriod()
{
$t1 = $this->createMock(TransportInterface::class);
$t1->expects($this->at(0))->method('send')->will($this->throwException(new TransportException()));
$t1->expects($this->once())->method('send');
$t2 = $this->createMock(TransportInterface::class);
$t2->expects($this->at(0))->method('send');
$t2->expects($this->at(1))->method('send');
$t2->expects($this->at(2))->method('send')->will($this->throwException(new TransportException()));
$t2->expects($this->exactly(3))->method('send');
$t = new FailoverTransport([$t1, $t2], 40);
$t->send(new RawMessage(''));
sleep(4);
$t->send(new RawMessage(''));
sleep(4);
$this->expectException(TransportException::class);
$this->expectExceptionMessage('All transports failed.');
$t->send(new RawMessage(''));
}

Expand All @@ -80,4 +135,15 @@ public function testSendOneDeadButRecover()
sleep(1);
$t->send(new RawMessage(''));
}

private function assertTransports(RoundRobinTransport $transport, int $cursor, array $deadTransports)
{
$p = new \ReflectionProperty(RoundRobinTransport::class, 'cursor');
$p->setAccessible(true);
$this->assertSame($cursor, $p->getValue($transport));

$p = new \ReflectionProperty(RoundRobinTransport::class, 'deadTransports');
$p->setAccessible(true);
$this->assertSame($deadTransports, iterator_to_array($p->getValue($transport)));
}
}
Expand Up @@ -36,8 +36,11 @@ public function testSendAlternate()
$t2->expects($this->once())->method('send');
$t = new RoundRobinTransport([$t1, $t2]);
$t->send(new RawMessage(''));
$this->assertTransports($t, 1, []);
$t->send(new RawMessage(''));
$this->assertTransports($t, 0, []);
$t->send(new RawMessage(''));
$this->assertTransports($t, 1, []);
}

public function testSendAllDead()
Expand All @@ -50,6 +53,7 @@ public function testSendAllDead()
$this->expectException(TransportException::class);
$this->expectExceptionMessage('All transports failed.');
$t->send(new RawMessage(''));
$this->assertTransports($t, 1, [$t1, $t2]);
}

public function testSendOneDead()
Expand All @@ -60,20 +64,57 @@ public function testSendOneDead()
$t2->expects($this->exactly(3))->method('send');
$t = new RoundRobinTransport([$t1, $t2]);
$t->send(new RawMessage(''));
$this->assertTransports($t, 0, [$t1]);
$t->send(new RawMessage(''));
$this->assertTransports($t, 0, [$t1]);
$t->send(new RawMessage(''));
$this->assertTransports($t, 0, [$t1]);
}

public function testSendOneDeadButRecover()
public function testSendOneDeadAndRecoveryNotWithinRetryPeriod()
{
$t1 = $this->createMock(TransportInterface::class);
$t1->expects($this->at(0))->method('send')->will($this->throwException(new TransportException()));
$t1->expects($this->at(1))->method('send');
$t1->expects($this->exactly(4))->method('send');
$t2 = $this->createMock(TransportInterface::class);
$t2->expects($this->once())->method('send');
$t = new RoundRobinTransport([$t1, $t2], 1);
$t2->expects($this->once())->method('send')->will($this->throwException(new TransportException()));
$t = new RoundRobinTransport([$t1, $t2], 60);
$t->send(new RawMessage(''));
$this->assertTransports($t, 1, []);
$t->send(new RawMessage(''));
sleep(2);
$this->assertTransports($t, 1, [$t2]);
$t->send(new RawMessage(''));
$this->assertTransports($t, 1, [$t2]);
$t->send(new RawMessage(''));
$this->assertTransports($t, 1, [$t2]);
}

public function testSendOneDeadAndRecoveryWithinRetryPeriod()
{
$t1 = $this->createMock(TransportInterface::class);
$t1->expects($this->exactly(3))->method('send');
$t2 = $this->createMock(TransportInterface::class);
$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);
$t->send(new RawMessage(''));
$this->assertTransports($t, 1, []);
$t->send(new RawMessage(''));
$this->assertTransports($t, 1, [$t2]);
sleep(5);
$t->send(new RawMessage(''));
$this->assertTransports($t, 0, []);
$t->send(new RawMessage(''));
$this->assertTransports($t, 1, []);
}

private function assertTransports(RoundRobinTransport $transport, int $cursor, array $deadTransports)
{
$p = new \ReflectionProperty($transport, 'cursor');
$p->setAccessible(true);
$this->assertSame($cursor, $p->getValue($transport));

$p = new \ReflectionProperty($transport, 'deadTransports');
$p->setAccessible(true);
$this->assertSame($deadTransports, iterator_to_array($p->getValue($transport)));
}
}
20 changes: 16 additions & 4 deletions src/Symfony/Component/Mailer/Transport/RoundRobinTransport.php
Expand Up @@ -29,6 +29,7 @@ class RoundRobinTransport implements TransportInterface
private $deadTransports;
private $transports = [];
private $retryPeriod;
private $cursor = 0;

/**
* @param TransportInterface[] $transports
Expand Down Expand Up @@ -62,26 +63,37 @@ public function send(RawMessage $message, SmtpEnvelope $envelope = null): ?SentM
*/
protected function getNextTransport(): ?TransportInterface
{
while ($transport = array_shift($this->transports)) {
$cursor = $this->cursor;
while (true) {
$transport = $this->transports[$cursor];

if (!$this->isTransportDead($transport)) {
break;
}

if ((microtime(true) - $this->deadTransports[$transport]) > $this->retryPeriod) {
$this->deadTransports->detach($transport);

break;
}
}

if ($transport) {
$this->transports[] = $transport;
if ($this->cursor === $cursor = $this->moveCursor($cursor)) {
return null;
}
}

$this->cursor = $this->moveCursor($cursor);

return $transport;
}

protected function isTransportDead(TransportInterface $transport): bool
{
return $this->deadTransports->contains($transport);
}

private function moveCursor(int $cursor): int
{
return ++$cursor >= \count($this->transports) ? 0 : $cursor;
}
}

0 comments on commit 751baaf

Please sign in to comment.