diogo/symfony
Archived
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

bug #31024 [Mailer] fixed roundrobin test one dead which should recover (scuben, fabpot)

Browse Source 
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
-------

5d4d4e7a71 fixed roundrobin dead transport which should recover
ccbb171312 fixed roundrobin dead transport which should recover
This commit is contained in:
Fabien Potencier 2019-04-11 08:24:11 +02:00
commit 751baaf3c7
3 changed files with 129 additions and 10 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

66
src/Symfony/Component/Mailer/Tests/Transport/FailoverTransportTest.php
View File

@ -14,6 +14,7 @@ namespace Symfony\Component\Mailer\Tests\Transport;
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;
@ -36,8 +37,11 @@ class FailoverTransportTest extends TestCase
$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()
@ -50,6 +54,7 @@ class FailoverTransportTest extends TestCase
$this->expectException(TransportException::class);
$this->expectExceptionMessage('All transports failed.');
$t->send(new RawMessage(''));
$this->assertTransports($t, 0, [$t1, $t2]);
}
public function testSendOneDead()
@ -60,7 +65,57 @@ class FailoverTransportTest extends TestCase
$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(''));
}
@ -80,4 +135,15 @@ class FailoverTransportTest extends TestCase
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)));
}
}

53
src/Symfony/Component/Mailer/Tests/Transport/RoundRobinTransportTest.php
View File

@ -36,8 +36,11 @@ class RoundRobinTransportTest extends TestCase
$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()
@ -50,6 +53,7 @@ class RoundRobinTransportTest extends TestCase
$this->expectException(TransportException::class);
$this->expectExceptionMessage('All transports failed.');
$t->send(new RawMessage(''));
$this->assertTransports($t, 1, [$t1, $t2]);
}
public function testSendOneDead()
@ -60,20 +64,57 @@ class RoundRobinTransportTest extends TestCase
$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(''));
sleep(2);
$this->assertTransports($t, 1, []);
$t->send(new RawMessage(''));
$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
src/Symfony/Component/Mailer/Transport/RoundRobinTransport.php
View File

@ -29,6 +29,7 @@ class RoundRobinTransport implements TransportInterface
private $deadTransports;
private $transports = [];
private $retryPeriod;
private $cursor = 0;
/**
* @param TransportInterface[] $transports
@ -62,20 +63,26 @@ class RoundRobinTransport implements TransportInterface
*/
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 ($this->cursor === $cursor = $this->moveCursor($cursor)) {
return null;
}
}
if ($transport) {
$this->transports[] = $transport;
}
$this->cursor = $this->moveCursor($cursor);
return $transport;
}
@ -84,4 +91,9 @@ class RoundRobinTransport implements TransportInterface
{
return $this->deadTransports->contains($transport);
}
private function moveCursor(int $cursor): int
{
return ++$cursor >= \count($this->transports) ? 0 : $cursor;
}
}