Skip to content

[Notifier] Make FailoverTransport always pick the first transport #41078

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
May 6, 2021
Merged

[Notifier] Make FailoverTransport always pick the first transport #41078

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
166 changes: 166 additions & 0 deletions src/Symfony/Component/Notifier/Tests/Transport/FailoverTransportTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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);
}
}
5 changes: 5 additions & 0 deletions src/Symfony/Component/Notifier/Transport/FailoverTransport.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,11 @@ protected function getNextTransport(MessageInterface $message): ?TransportInterf
return $this->currentTransport;
}

protected function getInitialCursor(): int
{
return 0;
}

protected function getNameSymbol(): string
{
return '||';
Expand Down
21 changes: 17 additions & 4 deletions src/Symfony/Component/Notifier/Transport/RoundRobinTransport.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ class RoundRobinTransport implements TransportInterface
private $deadTransports;
private $transports = [];
private $retryPeriod;
private $cursor = 0;
private $cursor = -1;

/**
* @param TransportInterface[] $transports
Expand All @@ -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
Expand All @@ -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);
Expand All @@ -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;
}

Expand Down Expand Up @@ -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 '&&';
Expand Down