8000 bug #49063 [Messenger] Respect `isRetryable` decision of the retry st… · symfony/symfony@f055c80 · GitHub
[go: up one dir, main page]
Skip to content

Commit f055c80

Browse files
fabpotfabpot
committed
bug #49063 [Messenger] Respect isRetryable decision of the retry strategy for re-delivery (FlyingDR)
This PR was merged into the 5.4 branch. Discussion ---------- [Messenger] Respect `isRetryable` decision of the retry strategy for re-delivery | Q | A | ------------- | --- | Branch? | 5.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | | License | MIT | Doc PR | [Documentation](https://symfony.com/doc/5.4/messenger.html#retries-failures) for retry strategy for the Messenger component declares that message will not be retried to be re-delivered more than the value of `max_retries` configuration for the retry strategy. However, after merging #36557 [actual implementation](https://github.com/symfony/symfony/blob/39cd93a9f7ea072b26853e87ceb1142d6dd019b0/src/Symfony/Component/Messenger/EventListener/SendFailedMessageForRetryListener.php#L126-L128) gives priority to the existence of the `RecoverableExceptionInterface`, effectively opening a way for a message to be re-delivered indefinitely. Additionally, in the case of using `multiplier` with a value of more than 1 this unlimited re-delivery causes unlimited growth of the `delay` value for the `DelayStamp`. Its type is d 8000 efined as `int`, so on 32-bit platforms after some time `delay` value may exceed `PHP_INT_MAX` which will lead to the `TypeError`. The proposed change enforces respect for the `max_retries` setting value for the decision of re-delivery. Commits ------- 8fc3dcc [Messenger] Respect `isRetryable` decision of the retry strategy when deciding if failed message should be re-delivered
2 parents 2caaddc + 8fc3dcc commit f055c80

File tree

2 files changed

+26
-4
lines changed

2 files changed

+26
-4
lines changed

‎src/Symfony/Component/Messenger/EventListener/SendFailedMessageForRetryListener.php

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -123,7 +123,8 @@ public static function getSubscribedEvents()
123123

124124
private function shouldRetry(\Throwable $e, Envelope $envelope, RetryStrategyInterface $retryStrategy): bool
125125
{
126-
if ($e instanceof RecoverableExceptionInterface) {
126+
$isRetryable = $retryStrategy->isRetryable($envelope, $e);
127+
if ($isRetryable && $e instanceof RecoverableExceptionInterface) {
127128
return true;
128129
}
129130

@@ -132,7 +133,7 @@ private function shouldRetry(\Throwable $e, Envelope $envelope, RetryStrategyInt
132133
if ($e instanceof HandlerFailedException) {
133134
$shouldNotRetry = true;
134135
foreach ($e->getNestedExceptions() as $nestedException) {
135-
if ($nestedException instanceof RecoverableExceptionInterface) {
136+
if ($isRetryable && $nestedException instanceof RecoverableExceptionInterface) {
136137
return true;
137138
}
138139

@@ -150,7 +151,7 @@ private function shouldRetry(\Throwable $e, Envelope $envelope, RetryStrategyInt
150151
return false;
151152
}
152153

153-
return $retryStrategy->isRetryable($envelope, $e);
154+
return $isRetryable;
154155
}
155156

156157
private function getRetryStrategyForTransport(string $alias): ?RetryStrategyInterface

‎src/Symfony/Component/Messenger/Tests/EventListener/SendFailedMessageForRetryListenerTest.php

Lines changed: 22 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -63,7 +63,7 @@ public function testRecoverableStrategyCausesRetry()
6363
$senderLocator->expects($this->once())->method('has')->willReturn(true);
6464
$senderLocator->expects($this->once())->method('get')->willReturn($sender);
6565
$retryStategy = $this->createMock(RetryStrategyInterface::class);
66-
$retryStategy->expects($this->never())->method('isRetryable');
66+
$retryStategy->expects($this->once())->method('isRetryable')->willReturn(true);
6767
$retryStategy->expects($this->once())->method('getWaitingTime')->willReturn(1000);
6868
$retryStrategyLocator = $this->createMock(ContainerInterface::class);
6969
$retryStrategyLocator->expects($this->once())->method('has')->willReturn(true);
@@ -78,6 +78,27 @@ public function testRecoverableStrategyCausesRetry()
7878
$listener->onMessageFailed($event);
7979
}
8080

81+
public function testRetryIsOnlyAllowedWhenPermittedByRetryStrategy()
82+
{
83+
$senderLocator = $this->createMock(ContainerInterface::class);
84+
$senderLocator->expects($this->never())->method('has');
85+
$senderLocator->expects($this->never())->method('get');
86+
$retryStrategy = $this->createMock(RetryStrategyInterface::class);
87+
$retryStrategy->expects($this->once())->method('isRetryable')->willReturn(false);
88+
$retryStrategy->expects($this->never())->method('getWaitingTime');
89+
$retryStrategyLocator = $this->createMock(ContainerInterface::class);
90+
$retryStrategyLocator->expects($this->once())->method('has')->willReturn(true);
91+
$retryStrategyLocator->expects($this->once())->method('get')->willReturn($retryStrategy);
92+
93+
$listener = new SendFailedMessageForRetryListener($senderLocator, $retryStrategyLocator);
94+
95+
$exception = new RecoverableMessageHandlingException('retry');
96+
$envelope = new Envelope(new \stdClass());
97+
$event = new WorkerMessageFailedEvent($envelope, 'my_receiver', $exception);
98+
99+
$listener->onMessageFailed($event);
100+
}
101+
81102
public function testEnvelopeIsSentToTransportOnRetry()
82103
{
83104
$exception = new \Exception('no!');

0 commit comments

Comments
 (0)
0