From ccbb171312ccde082603781bb1d863b0169d3186 Mon Sep 17 00:00:00 2001 From: Patrick Landolt Date: Mon, 8 Apr 2019 17:57:55 +0200 Subject: [PATCH 1/2] fixed roundrobin dead transport which should recover --- .../Tests/Transport/FailoverTransportTest.php | 63 +++++++++++++++++++ .../Transport/RoundRobinTransportTest.php | 29 +++++++-- .../Mailer/Transport/RoundRobinTransport.php | 9 +++ 3 files changed, 96 insertions(+), 5 deletions(-) diff --git a/src/Symfony/Component/Mailer/Tests/Transport/FailoverTransportTest.php b/src/Symfony/Component/Mailer/Tests/Transport/FailoverTransportTest.php index dc3bc21a7d..031dc32edb 100644 --- a/src/Symfony/Component/Mailer/Tests/Transport/FailoverTransportTest.php +++ b/src/Symfony/Component/Mailer/Tests/Transport/FailoverTransportTest.php @@ -64,6 +64,69 @@ class FailoverTransportTest extends TestCase $t->send(new RawMessage('')); } + public function testSendOneDeadAndRecoveryNotWithinRetryPeriod() + { + $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->exactly(5))->method('send'); + $t = new FailoverTransport([$t1, $t2], 40); + $t->send(new RawMessage('')); + sleep(4); + $t->send(new RawMessage('')); + sleep(4); + $t->send(new RawMessage('')); + sleep(4); + $t->send(new RawMessage('')); + sleep(4); + $t->send(new RawMessage('')); + } + + 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'); + $t1->expects($this->exactly(3))->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())); + $t2->expects($this->exactly(4))->method('send'); + $t = new FailoverTransport([$t1, $t2], 6); + $t->send(new RawMessage('')); // t1>fail - t2>sent + sleep(4); + $t->send(new RawMessage('')); // t2>sent + sleep(4); + $t->send(new RawMessage('')); // t2>sent + sleep(4); + $t->send(new RawMessage('')); // t2>fail - t1>sent + sleep(4); + $t->send(new RawMessage('')); // t1>sent + } + + 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('')); + } + public function testSendOneDeadButRecover() { $t1 = $this->createMock(TransportInterface::class); diff --git a/src/Symfony/Component/Mailer/Tests/Transport/RoundRobinTransportTest.php b/src/Symfony/Component/Mailer/Tests/Transport/RoundRobinTransportTest.php index b27a3e7949..7acbe9c483 100644 --- a/src/Symfony/Component/Mailer/Tests/Transport/RoundRobinTransportTest.php +++ b/src/Symfony/Component/Mailer/Tests/Transport/RoundRobinTransportTest.php @@ -64,16 +64,35 @@ class RoundRobinTransportTest extends TestCase $t->send(new RawMessage('')); } - 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->at(0))->method('send')->will($this->throwException(new TransportException())); $t2->expects($this->once())->method('send'); - $t = new RoundRobinTransport([$t1, $t2], 1); + $t = new RoundRobinTransport([$t1, $t2], 60); $t->send(new RawMessage('')); - sleep(2); + $t->send(new RawMessage('')); + $t->send(new RawMessage('')); + $t->send(new RawMessage('')); + } + + 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'); + $t2->expects($this->exactly(2))->method('send'); + $t = new RoundRobinTransport([$t1, $t2], 3); + $t->send(new RawMessage('')); + sleep(5); + $t->send(new RawMessage('')); + sleep(5); + $t->send(new RawMessage('')); + sleep(5); $t->send(new RawMessage('')); } } diff --git a/src/Symfony/Component/Mailer/Transport/RoundRobinTransport.php b/src/Symfony/Component/Mailer/Transport/RoundRobinTransport.php index 22b1ba9714..e585e3635e 100644 --- a/src/Symfony/Component/Mailer/Transport/RoundRobinTransport.php +++ b/src/Symfony/Component/Mailer/Transport/RoundRobinTransport.php @@ -66,11 +66,20 @@ class RoundRobinTransport implements TransportInterface 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->deadTransports->count() >= \count($this->transports)) { + return null; + } } if ($transport) { From 5d4d4e7a712cd222965e4e891a6c7737fb3b0cbb Mon Sep 17 00:00:00 2001 From: Fabien Potencier Date: Wed, 10 Apr 2019 09:39:31 +0200 Subject: [PATCH 2/2] fixed roundrobin dead transport which should recover --- .../Tests/Transport/FailoverTransportTest.php | 45 ++++++++++--------- .../Transport/RoundRobinTransportTest.php | 34 +++++++++++--- .../Mailer/Transport/RoundRobinTransport.php | 21 +++++---- 3 files changed, 64 insertions(+), 36 deletions(-) diff --git a/src/Symfony/Component/Mailer/Tests/Transport/FailoverTransportTest.php b/src/Symfony/Component/Mailer/Tests/Transport/FailoverTransportTest.php index 031dc32edb..9243263fdd 100644 --- a/src/Symfony/Component/Mailer/Tests/Transport/FailoverTransportTest.php +++ b/src/Symfony/Component/Mailer/Tests/Transport/FailoverTransportTest.php @@ -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,27 +65,11 @@ 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('')); - } - - public function testSendOneDeadAndRecoveryNotWithinRetryPeriod() - { - $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->exactly(5))->method('send'); - $t = new FailoverTransport([$t1, $t2], 40); - $t->send(new RawMessage('')); - sleep(4); - $t->send(new RawMessage('')); - sleep(4); - $t->send(new RawMessage('')); - sleep(4); - $t->send(new RawMessage('')); - sleep(4); - $t->send(new RawMessage('')); + $this->assertTransports($t, 0, [$t1]); } public function testSendOneDeadAndRecoveryWithinRetryPeriod() @@ -88,23 +77,26 @@ class FailoverTransportTest extends TestCase $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(3))->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())); - $t2->expects($this->exactly(4))->method('send'); $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() @@ -143,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))); + } } diff --git a/src/Symfony/Component/Mailer/Tests/Transport/RoundRobinTransportTest.php b/src/Symfony/Component/Mailer/Tests/Transport/RoundRobinTransportTest.php index 7acbe9c483..4b2316da5d 100644 --- a/src/Symfony/Component/Mailer/Tests/Transport/RoundRobinTransportTest.php +++ b/src/Symfony/Component/Mailer/Tests/Transport/RoundRobinTransportTest.php @@ -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,8 +64,11 @@ 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 testSendOneDeadAndRecoveryNotWithinRetryPeriod() @@ -69,13 +76,16 @@ class RoundRobinTransportTest extends TestCase $t1 = $this->createMock(TransportInterface::class); $t1->expects($this->exactly(4))->method('send'); $t2 = $this->createMock(TransportInterface::class); - $t2->expects($this->at(0))->method('send')->will($this->throwException(new TransportException())); - $t2->expects($this->once())->method('send'); + $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('')); + $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() @@ -85,14 +95,26 @@ class RoundRobinTransportTest extends TestCase $t2 = $this->createMock(TransportInterface::class); $t2->expects($this->at(0))->method('send')->will($this->throwException(new TransportException())); $t2->expects($this->at(1))->method('send'); - $t2->expects($this->exactly(2))->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('')); - sleep(5); - $t->send(new RawMessage('')); - sleep(5); + $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))); } } diff --git a/src/Symfony/Component/Mailer/Transport/RoundRobinTransport.php b/src/Symfony/Component/Mailer/Transport/RoundRobinTransport.php index e585e3635e..928b6c06ad 100644 --- a/src/Symfony/Component/Mailer/Transport/RoundRobinTransport.php +++ b/src/Symfony/Component/Mailer/Transport/RoundRobinTransport.php @@ -29,6 +29,7 @@ class RoundRobinTransport implements TransportInterface private $deadTransports; private $transports = []; private $retryPeriod; + private $cursor = 0; /** * @param TransportInterface[] $transports @@ -62,7 +63,10 @@ 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; } @@ -73,18 +77,12 @@ class RoundRobinTransport implements TransportInterface break; } - if ($transport) { - $this->transports[] = $transport; - } - - if ($this->deadTransports->count() >= \count($this->transports)) { + if ($this->cursor === $cursor = $this->moveCursor($cursor)) { return null; } } - if ($transport) { - $this->transports[] = $transport; - } + $this->cursor = $this->moveCursor($cursor); return $transport; } @@ -93,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; + } }