Skip to content

Commit 274babb

Browse files
fabpotfabpot
committed
fixed roundrobin dead transport which should recover
1 parent c55fd56 commit 274babb

File tree

6 files changed

+126
-70
lines changed

6 files changed

+126
-70
lines changed

‎FailoverTransportTest.php

Lines changed: 24 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414
use PHPUnit\Framework\TestCase;
1515
use Symfony\Component\Mailer\Exception\TransportException;
1616
use Symfony\Component\Mailer\Transport\FailoverTransport;
17+
use Symfony\Component\Mailer\Transport\RoundRobinTransport;
1718
use Symfony\Component\Mailer\Transport\TransportInterface;
1819
use Symfony\Component\Mime\RawMessage;
1920

@@ -36,8 +37,11 @@ public function testSendFirstWork()
3637
$t2->expects($this->never())->method('send');
3738
$t = new FailoverTransport([$t1, $t2]);
3839
$t->send(new RawMessage(''));
40+
$this->assertTransports($t, 1, []);
3941
$t->send(new RawMessage(''));
42+
$this->assertTransports($t, 1, []);
4043
$t->send(new RawMessage(''));
44+
$this->assertTransports($t, 1, []);
4145
}
4246

4347
public function testSendAllDead()
@@ -50,6 +54,7 @@ public function testSendAllDead()
5054
$this->expectException(TransportException::class);
5155
$this->expectExceptionMessage('All transports failed.');
5256
$t->send(new RawMessage(''));
57+
$this->assertTransports($t, 0, [$t1, $t2]);
5358
}
5459

5560
public function testSendOneDead()
@@ -60,51 +65,38 @@ public function testSendOneDead()
6065
$t2->expects($this->exactly(3))->method('send');
6166
$t = new FailoverTransport([$t1, $t2]);
6267
$t->send(new RawMessage(''));
68+
$this->assertTransports($t, 0, [$t1]);
6369
$t->send(new RawMessage(''));
70+
$this->assertTransports($t, 0, [$t1]);
6471
$t->send(new RawMessage(''));
65-
}
66-
67-
public function testSendOneDeadAndRecoveryNotWithinRetryPeriod()
68-
{
69-
$t1 = $this->createMock(TransportInterface::class);
70-
$t1->expects($this->at(0))->method('send')->will($this->throwException(new TransportException()));
71-
$t1->expects($this->once())->method('send');
72-
$t2 = $this->createMock(TransportInterface::class);
73-
$t2->expects($this->exactly(5))->method('send');
74-
$t = new FailoverTransport([$t1, $t2], 40);
75-
$t->send(new RawMessage(''));
76-
sleep(4);
77-
$t->send(new RawMessage(''));
78-
sleep(4);
79-
$t->send(new RawMessage(''));
80-
sleep(4);
81-
$t->send(new RawMessage(''));
82-
sleep(4);
83-
$t->send(new RawMessage(''));
72+
$this->assertTransports($t, 0, [$t1]);
8473
}
8574

8675
public function testSendOneDeadAndRecoveryWithinRetryPeriod()
8776
{
8877
$t1 = $this->createMock(TransportInterface::class);
8978
$t1->expects($this->at(0))->method('send')->will($this->throwException(new TransportException()));
9079
$t1->expects($this->at(1))->method('send');
91-
$t1->expects($this->exactly(3))->method('send');
9280
$t2 = $this->createMock(TransportInterface::class);
9381
$t2->expects($this->at(0))->method('send');
9482
$t2->expects($this->at(1))->method('send');
9583
$t2->expects($this->at(2))->method('send');
9684
$t2->expects($this->at(3))->method('send')->will($this->throwException(new TransportException()));
97-
$t2->expects($this->exactly(4))->method('send');
9885
$t = new FailoverTransport([$t1, $t2], 6);
9986
$t->send(new RawMessage('')); // t1>fail - t2>sent
87+
$this->assertTransports($t, 0, [$t1]);
10088
sleep(4);
10189
$t->send(new RawMessage('')); // t2>sent
90+
$this->assertTransports($t, 0, [$t1]);
10291
sleep(4);
10392
$t->send(new RawMessage('')); // t2>sent
93+
$this->assertTransports($t, 0, [$t1]);
10494
sleep(4);
10595
$t->send(new RawMessage('')); // t2>fail - t1>sent
96+
$this->assertTransports($t, 1, [$t2]);
10697
sleep(4);
10798
$t->send(new RawMessage('')); // t1>sent
99+
$this->assertTransports($t, 1, [$t2]);
108100
}
109101

110102
public function testSendAllDeadWithinRetryPeriod()
@@ -143,4 +135,15 @@ public function testSendOneDeadButRecover()
143135
sleep(1);
144136
$t->send(new RawMessage(''));
145137
}
138+
139+
private function assertTransports(RoundRobinTransport $transport, int $cursor, array $deadTransports)
140+
{
141+
$p = new \ReflectionProperty(RoundRobinTransport::class, 'cursor');
142+
$p->setAccessible(true);
143+
$this->assertSame($cursor, $p->getValue($transport));
144+
145+
$p = new \ReflectionProperty(RoundRobinTransport::class, 'deadTransports');
146+
$p->setAccessible(true);
147+
$this->assertSame($deadTransports, iterator_to_array($p->getValue($transport)));
148+
}
146149
}

‎RoundRobinTransport.php

Lines changed: 12 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@ class RoundRobinTransport implements TransportInterface
2929
private $deadTransports;
3030
private $transports = [];
3131
private $retryPeriod;
32+
private $cursor = 0;
3233

3334
/**
3435
* @param TransportInterface[] $transports
@@ -62,7 +63,10 @@ public function send(RawMessage $message, SmtpEnvelope $envelope = null): ?SentM
6263
*/
6364
protected function getNextTransport(): ?TransportInterface
6465
{
65-
while ($transport = array_shift($this->transports)) {
66+
$cursor = $this->cursor;
67+
while (true) {
68+
$transport = $this->transports[$cursor];
69+
6670
if (!$this->isTransportDead($transport)) {
6771
break;
6872
}
@@ -73,18 +77,12 @@ protected function getNextTransport(): ?TransportInterface
7377
break;
7478
}
7579

76-
if ($transport) {
77-
$this->transports[] = $transport;
78-
}
79-
80-
if ($this->deadTransports->count() >= \count($this->transports)) {
80+
if ($this->cursor === $cursor = $this->moveCursor($cursor)) {
8181
return null;
8282
}
8383
}
8484

85-
if ($transport) {
86-
$this->transports[] = $transport;
87-
}
85+
$this->cursor = $this->moveCursor($cursor);
8886

8987
return $transport;
9088
}
@@ -93,4 +91,9 @@ protected function isTransportDead(TransportInterface $transport): bool
9391
{
9492
return $this->deadTransports->contains($transport);
9593
}
94+
95+
private function moveCursor(int $cursor): int
96+
{
97+
return ++$cursor >= \count($this->transports) ? 0 : $cursor;
98+
}
9699
}

‎RoundRobinTransportTest.php

Lines changed: 27 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -36,8 +36,11 @@ public function testSendAlternate()
3636
$t2->expects($this->once())->method('send');
3737
$t = new RoundRobinTransport([$t1, $t2]);
3838
$t->send(new RawMessage(''));
39+
$this->assertTransports($t, 1, []);
3940
$t->send(new RawMessage(''));
41+
$this->assertTransports($t, 0, []);
4042
$t->send(new RawMessage(''));
43+
$this->assertTransports($t, 1, []);
4144
}
4245

4346
public function testSendAllDead()
@@ -50,6 +53,7 @@ public function testSendAllDead()
5053
$this->expectException(TransportException::class);
5154
$this->expectExceptionMessage('All transports failed.');
5255
$t->send(new RawMessage(''));
56+
$this->assertTransports($t, 1, [$t1, $t2]);
5357
}
5458

5559
public function testSendOneDead()
@@ -60,22 +64,28 @@ public function testSendOneDead()
6064
$t2->expects($this->exactly(3))->method('send');
6165
$t = new RoundRobinTransport([$t1, $t2]);
6266
$t->send(new RawMessage(''));
67+
$this->assertTransports($t, 0, [$t1]);
6368
$t->send(new RawMessage(''));
69+
$this->assertTransports($t, 0, [$t1]);
6470
$t->send(new RawMessage(''));
71+
$this->assertTransports($t, 0, [$t1]);
6572
}
6673

6774
public function testSendOneDeadAndRecoveryNotWithinRetryPeriod()
6875
{
6976
$t1 = $this->createMock(TransportInterface::class);
7077
$t1->expects($this->exactly(4))->method('send');
7178
$t2 = $this->createMock(TransportInterface::class);
72-
$t2->expects($this->at(0))->method('send')->will($this->throwException(new TransportException()));
73-
$t2->expects($this->once())->method('send');
79+
$t2->expects($this->once())->method('send')->will($this->throwException(new TransportException()));
7480
$t = new RoundRobinTransport([$t1, $t2], 60);
7581
$t->send(new RawMessage(''));
82+
$this->assertTransports($t, 1, []);
7683
$t->send(new RawMessage(''));
84+
$this->assertTransports($t, 1, [$t2]);
7785
$t->send(new RawMessage(''));
86+
$this->assertTransports($t, 1, [$t2]);
7887
$t->send(new RawMessage(''));
88+
$this->assertTransports($t, 1, [$t2]);
7989
}
8090

8191
public function testSendOneDeadAndRecoveryWithinRetryPeriod()
@@ -85,14 +95,26 @@ public function testSendOneDeadAndRecoveryWithinRetryPeriod()
8595
$t2 = $this->createMock(TransportInterface::class);
8696
$t2->expects($this->at(0))->method('send')->will($this->throwException(new TransportException()));
8797
$t2->expects($this->at(1))->method('send');
88-
$t2->expects($this->exactly(2))->method('send');
8998
$t = new RoundRobinTransport([$t1, $t2], 3);
9099
$t->send(new RawMessage(''));
91-
sleep(5);
100+
$this->assertTransports($t, 1, []);
92101
$t->send(new RawMessage(''));
102+
$this->assertTransports($t, 1, [$t2]);
93103
sleep(5);
94104
$t->send(new RawMessage(''));
95-
sleep(5);
105+
$this->assertTransports($t, 0, []);
96106
$t->send(new RawMessage(''));
107+
$this->assertTransports($t, 1, []);
108+
}
109+
110+
private function assertTransports(RoundRobinTransport $transport, int $cursor, array $deadTransports)
111+
{
112+
$p = new \ReflectionProperty($transport, 'cursor');
113+
$p->setAccessible(true);
114+
$this->assertSame($cursor, $p->getValue($transport));
115+
116+
$p = new \ReflectionProperty($transport, 'deadTransports');
117+
$p->setAccessible(true);
118+
$this->assertSame($deadTransports, iterator_to_array($p->getValue($transport)));
97119
}
98120
}

‎Tests/Transport/FailoverTransportTest.php

Lines changed: 24 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414
use PHPUnit\Framework\TestCase;
1515
use Symfony\Component\Mailer\Exception\TransportException;
1616
use Symfony\Component\Mailer\Transport\FailoverTransport;
17+
use Symfony\Component\Mailer\Transport\RoundRobinTransport;
1718
use Symfony\Component\Mailer\Transport\TransportInterface;
1819
use Symfony\Component\Mime\RawMessage;
1920

@@ -36,8 +37,11 @@ public function testSendFirstWork()
3637
$t2->expects($this->never())->method('send');
3738
$t = new FailoverTransport([$t1, $t2]);
3839
$t->send(new RawMessage(''));
40+
$this->assertTransports($t, 1, []);
3941
$t->send(new RawMessage(''));
42+
$this->assertTransports($t, 1, []);
4043
$t->send(new RawMessage(''));
44+
$this->assertTransports($t, 1, []);
4145
}
4246

4347
public function testSendAllDead()
@@ -50,6 +54,7 @@ public function testSendAllDead()
5054
$this->expectException(TransportException::class);
5155
$this->expectExceptionMessage('All transports failed.');
5256
$t->send(new RawMessage(''));
57+
$this->assertTransports($t, 0, [$t1, $t2]);
5358
}
5459

5560
public function testSendOneDead()
@@ -60,51 +65,38 @@ public function testSendOneDead()
6065
$t2->expects($this->exactly(3))->method('send');
6166
$t = new FailoverTransport([$t1, $t2]);
6267
$t->send(new RawMessage(''));
68+
$this->assertTransports($t, 0, [$t1]);
6369
$t->send(new RawMessage(''));
70+
$this->assertTransports($t, 0, [$t1]);
6471
$t->send(new RawMessage(''));
65-
}
66-
67-
public function testSendOneDeadAndRecoveryNotWithinRetryPeriod()
68-
{
69-
$t1 = $this->createMock(TransportInterface::class);
70-
$t1->expects($this->at(0))->method('send')->will($this->throwException(new TransportException()));
71-
$t1->expects($this->once())->method('send');
72-
$t2 = $this->createMock(TransportInterface::class);
73-
$t2->expects($this->exactly(5))->method('send');
74-
$t = new FailoverTransport([$t1, $t2], 40);
75-
$t->send(new RawMessage(''));
76-
sleep(4);
77-
$t->send(new RawMessage(''));
78-
sleep(4);
79-
$t->send(new RawMessage(''));
80-
sleep(4);
81-
$t->send(new RawMessage(''));
82-
sleep(4);
83-
$t->send(new RawMessage(''));
72+
$this->assertTransports($t, 0, [$t1]);
8473
}
8574

8675
public function testSendOneDeadAndRecoveryWithinRetryPeriod()
8776
{
8877
$t1 = $this->createMock(TransportInterface::class);
8978
$t1->expects($this->at(0))->method('send')->will($this->throwException(new TransportException()));
9079
$t1->expects($this->at(1))->method('send');
91-
$t1->expects($this->exactly(3))->method('send');
9280
$t2 = $this->createMock(TransportInterface::class);
9381
$t2->expects($this->at(0))->method('send');
9482
$t2->expects($this->at(1))->method('send');
9583
$t2->expects($this->at(2))->method('send');
9684
$t2->expects($this->at(3))->method('send')->will($this->throwException(new TransportException()));
97-
$t2->expects($this->exactly(4))->method('send');
9885
$t = new FailoverTransport([$t1, $t2], 6);
9986
$t->send(new RawMessage('')); // t1>fail - t2>sent
87+
$this->assertTransports($t, 0, [$t1]);
10088
sleep(4);
10189
$t->send(new RawMessage('')); // t2>sent
90+
$this->assertTransports($t, 0, [$t1]);
10291
sleep(4);
10392
$t->send(new RawMessage('')); // t2>sent
93+
$this->assertTransports($t, 0, [$t1]);
10494
sleep(4);
10595
$t->send(new RawMessage('')); // t2>fail - t1>sent
96+
$this->assertTransports($t, 1, [$t2]);
10697
sleep(4);
10798
$t->send(new RawMessage('')); // t1>sent
99+
$this->assertTransports($t, 1, [$t2]);
108100
}
109101

110102
public function testSendAllDeadWithinRetryPeriod()
@@ -143,4 +135,15 @@ public function testSendOneDeadButRecover()
143135
sleep(1);
144136
$t->send(new RawMessage(''));
145137
}
138+
139+
private function assertTransports(RoundRobinTransport $transport, int $cursor, array $deadTransports)
140+
{
141+
$p = new \ReflectionProperty(RoundRobinTransport::class, 'cursor');
142+
$p->setAccessible(true);
143+
$this->assertSame($cursor, $p->getValue($transport));
144+
145+
$p = new \ReflectionProperty(RoundRobinTransport::class, 'deadTransports');
146+
$p->setAccessible(true);
147+
$this->assertSame($deadTransports, iterator_to_array($p->getValue($transport)));
148+
}
146149
}

0 commit comments

Comments
 (0)