bug #41078 [Notifier] Make FailoverTransport always pick the first tr…

…ansport (jschaedl)

This PR was merged into the 5.2 branch.

Discussion
----------

[Notifier] Make FailoverTransport always pick the first transport

| Q             | A
| ------------- | ---
| Branch?       | 5.2
| Bug fix?      | yes
| New feature?  | no <!-- please update src/**/CHANGELOG.md files -->
| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tickets       | Fix #40895 <!-- prefix each issue number with "Fix #", no need to create an issue if none exist, explain below instead -->
| License       | MIT
| Doc PR        | - <!-- required for new features -->
<!--
Replace this notice by a short README for your feature/bugfix. This will help people
understand your PR and can be used as a start for the documentation.

Additionally (see https://symfony.com/releases):
 - Always add tests and ensure they pass.
 - Never break backward compatibility (see https://symfony.com/bc).
 - Bug fixes must be submitted against the lowest maintained branch where they apply
   (lowest branches are regularly merged to upper ones so they get the fixes too.)
 - Features and deprecations must be submitted against branch 5.x.
 - Changelog entry should follow https://symfony.com/doc/current/contributing/code/conventions.html#writing-a-changelog-entry
-->

Commits
-------

337f828 Make FailoverTransport always pick the first transport

src/Symfony/Component/Notifier/Tests/Transport/FailoverTransportTest.php

@@ -0,0 +1,166 @@
+<?php
+
+/*
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <fabien@symfony.com>
+ *
+ * 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);
+    }
+}

src/Symfony/Component/Notifier/Transport/FailoverTransport.php

@@ -33,6 +33,11 @@ protected function getNextTransport(MessageInterface $message): ?TransportInterf
         return $this->currentTransport;
     }
 
+    protected function getInitialCursor(): int
+    {
+        return 0;
+    }
+
     protected function getNameSymbol(): string
     {
         return '||';

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 @@ public function __construct(array $transports, int $retryPeriod = 60)
         $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 @@ public function supports(MessageInterface $message): bool
 
     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 @@ public function send(MessageInterface $message): SentMessage
      */
     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 @@ protected function isTransportDead(TransportInterface $transport): bool
         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 '&&';