From 337f828b2362fddcb4663b335848f03c2c821d30 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Sch=C3=A4dlich?= Date: Fri, 23 Apr 2021 06:58:19 +0100 Subject: [PATCH] Make FailoverTransport always pick the first transport --- .../Tests/Transport/FailoverTransportTest.php | 166 ++++++++++++++++++ .../Notifier/Transport/FailoverTransport.php | 5 + .../Transport/RoundRobinTransport.php | 21 ++- 3 files changed, 188 insertions(+), 4 deletions(-) create mode 100644 src/Symfony/Component/Notifier/Tests/Transport/FailoverTransportTest.php diff --git a/src/Symfony/Component/Notifier/Tests/Transport/FailoverTransportTest.php b/src/Symfony/Component/Notifier/Tests/Transport/FailoverTransportTest.php new file mode 100644 index 0000000000..2b48c20e20 --- /dev/null +++ b/src/Symfony/Component/Notifier/Tests/Transport/FailoverTransportTest.php @@ -0,0 +1,166 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace Symfony\Component\Notifier\Tests\Transport; + +use PHPUnit\Framework\TestCase; +use Symfony\Component\Notifier\Exception\LogicException; +use Symfony\Component\Notifier\Exception\RuntimeException; +use Symfony\Component\Notifier\Exception\TransportExceptionInterface; +use Symfony\Component\Notifier\Message\SentMessage; +use Symfony\Component\Notifier\Transport\FailoverTransport; +use Symfony\Component\Notifier\Transport\TransportInterface; + +/** + * @group time-sensitive + */ +class FailoverTransportTest extends TestCase +{ + public function testSendNoTransports() + { + $this->expectException(LogicException::class); + + new FailoverTransport([]); + } + + public function testToString() + { + $t1 = $this->createMock(TransportInterface::class); + $t1->expects($this->once())->method('__toString')->willReturn('t1://local'); + + $t2 = $this->createMock(TransportInterface::class); + $t2->expects($this->once())->method('__toString')->willReturn('t2://local'); + + $t = new FailoverTransport([$t1, $t2]); + + $this->assertEquals('t1://local || t2://local', (string) $t); + } + + public function testSendMessageNotSupportedByAnyTransport() + { + $t1 = $this->createMock(TransportInterface::class); + $t2 = $this->createMock(TransportInterface::class); + + $t = new FailoverTransport([$t1, $t2]); + + $this->expectException(LogicException::class); + + $t->send(new DummyMessage()); + } + + public function testSendFirstWork() + { + $message = new DummyMessage(); + + $t1 = $this->createMock(TransportInterface::class); + $t1->method('supports')->with($message)->willReturn(true); + $t1->expects($this->exactly(3))->method('send')->with($message)->willReturn(new SentMessage($message, 'test')); + + $t2 = $this->createMock(TransportInterface::class); + $t2->expects($this->never())->method('send'); + + $t = new FailoverTransport([$t1, $t2]); + + $t->send($message); + $t->send($message); + $t->send($message); + } + + public function testSendAllDead() + { + $message = new DummyMessage(); + + $t1 = $this->createMock(TransportInterface::class); + $t1->method('supports')->with($message)->willReturn(true); + $t1->expects($this->once())->method('send')->with($message)->will($this->throwException($this->createMock(TransportExceptionInterface::class))); + + $t2 = $this->createMock(TransportInterface::class); + $t2->method('supports')->with($message)->willReturn(true); + $t2->expects($this->once())->method('send')->with($message)->will($this->throwException($this->createMock(TransportExceptionInterface::class))); + + $t = new FailoverTransport([$t1, $t2]); + + $this->expectException(RuntimeException::class); + $this->expectExceptionMessage('All transports failed.'); + + $t->send($message); + } + + public function testSendOneDead() + { + $message = new DummyMessage(); + + $t1 = $this->createMock(TransportInterface::class); + $t1->method('supports')->with($message)->willReturn(true); + $t1->expects($this->once())->method('send')->will($this->throwException($this->createMock(TransportExceptionInterface::class))); + + $t2 = $this->createMock(TransportInterface::class); + $t2->method('supports')->with($message)->willReturn(true); + $t2->expects($this->exactly(1))->method('send')->with($message)->willReturn(new SentMessage($message, 'test')); + + $t = new FailoverTransport([$t1, $t2]); + + $t->send($message); + } + + public function testSendAllDeadWithinRetryPeriod() + { + $message = new DummyMessage(); + + $t1 = $this->createMock(TransportInterface::class); + $t1->method('supports')->with($message)->willReturn(true); + $t1->method('send')->will($this->throwException($this->createMock(TransportExceptionInterface::class))); + $t1->expects($this->once())->method('send'); + $t2 = $this->createMock(TransportInterface::class); + $t2->method('supports')->with($message)->willReturn(true); + $t2->expects($this->exactly(3)) + ->method('send') + ->willReturnOnConsecutiveCalls( + new SentMessage($message, 't2'), + new SentMessage($message, 't2'), + $this->throwException($this->createMock(TransportExceptionInterface::class)) + ); + $t = new FailoverTransport([$t1, $t2], 40); + $t->send($message); + sleep(4); + $t->send($message); + sleep(4); + + $this->expectException(RuntimeException::class); + $this->expectExceptionMessage('All transports failed.'); + + $t->send($message); + } + + public function testSendOneDeadButRecover() + { + $message = new DummyMessage(); + + $t1 = $this->createMock(TransportInterface::class); + $t1->method('supports')->with($message)->willReturn(true); + $t1->expects($this->exactly(2))->method('send')->willReturnOnConsecutiveCalls( + $this->throwException($this->createMock(TransportExceptionInterface::class)), + new SentMessage($message, 't1') + ); + $t2 = $this->createMock(TransportInterface::class); + $t2->method('supports')->with($message)->willReturn(true); + $t2->expects($this->exactly(2))->method('send')->willReturnOnConsecutiveCalls( + new SentMessage($message, 't2'), + $this->throwException($this->createMock(TransportExceptionInterface::class)) + ); + + $t = new FailoverTransport([$t1, $t2], 1); + + $t->send($message); + sleep(2); + $t->send($message); + } +} diff --git a/src/Symfony/Component/Notifier/Transport/FailoverTransport.php b/src/Symfony/Component/Notifier/Transport/FailoverTransport.php index 68edb9f118..23a13154d1 100644 --- a/src/Symfony/Component/Notifier/Transport/FailoverTransport.php +++ b/src/Symfony/Component/Notifier/Transport/FailoverTransport.php @@ -33,6 +33,11 @@ class FailoverTransport extends RoundRobinTransport return $this->currentTransport; } + protected function getInitialCursor(): int + { + return 0; + } + protected function getNameSymbol(): string { return '||'; diff --git a/src/Symfony/Component/Notifier/Transport/RoundRobinTransport.php b/src/Symfony/Component/Notifier/Transport/RoundRobinTransport.php index a67bccb95c..cfc544a4c3 100644 --- a/src/Symfony/Component/Notifier/Transport/RoundRobinTransport.php +++ b/src/Symfony/Component/Notifier/Transport/RoundRobinTransport.php @@ -29,7 +29,7 @@ class RoundRobinTransport implements TransportInterface private $deadTransports; private $transports = []; private $retryPeriod; - private $cursor = 0; + private $cursor = -1; /** * @param TransportInterface[] $transports @@ -43,9 +43,6 @@ class RoundRobinTransport implements TransportInterface $this->transports = $transports; $this->deadTransports = new \SplObjectStorage(); $this->retryPeriod = $retryPeriod; - // 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($transports) - 1); } public function __toString(): string @@ -66,6 +63,10 @@ class RoundRobinTransport implements TransportInterface public function send(MessageInterface $message): SentMessage { + if (!$this->supports($message)) { + throw new LogicException(sprintf('None of the configured Transports of "%s" supports the given message.', static::class)); + } + while ($transport = $this->getNextTransport($message)) { try { return $transport->send($message); @@ -82,12 +83,17 @@ class RoundRobinTransport implements TransportInterface */ protected function getNextTransport(MessageInterface $message): ?TransportInterface { + if (-1 === $this->cursor) { + $this->cursor = $this->getInitialCursor(); + } + $cursor = $this->cursor; while (true) { $transport = $this->transports[$cursor]; if (!$transport->supports($message)) { $cursor = $this->moveCursor($cursor); + continue; } @@ -116,6 +122,13 @@ class RoundRobinTransport implements TransportInterface return $this->deadTransports->contains($transport); } + protected function getInitialCursor(): int + { + // the cursor initial value is randomized so that + // when are not in a daemon, we are still rotating the transports + return mt_rand(0, \count($this->transports) - 1); + } + protected function getNameSymbol(): string { return '&&';