Skip to content

[Messenger] Allow fine-grained control in log severity of failures #44120

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

Closed
wants to merge 1 commit into from
Closed

[Messenger] Allow fine-grained control in log severity of failures #44120

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
47 changes: 9 additions & 38 deletions src/Symfony/Component/Messenger/EventListener/SendFailedMessageForRetryListener.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,14 +12,12 @@

use Psr\Container\ContainerInterface;
use Psr\Log\LoggerInterface;
use Psr\Log\LogLevel;
use Symfony\Component\EventDispatcher\EventSubscriberInterface;
use Symfony\Component\Messenger\Envelope;
use Symfony\Component\Messenger\Event\WorkerMessageFailedEvent;
use Symfony\Component\Messenger\Event\WorkerMessageRetriedEvent;
use Symfony\Component\Messenger\Exception\HandlerFailedException;
use Symfony\Component\Messenger\Exception\RecoverableExceptionInterface;
use Symfony\Component\Messenger\Exception\RuntimeException;
use Symfony\Component\Messenger\Exception\UnrecoverableExceptionInterface;
use Symfony\Component\Messenger\Retry\RetryStrategyInterface;
use Symfony\Component\Messenger\Stamp\DelayStamp;
use Symfony\Component\Messenger\Stamp\RedeliveryStamp;
Expand Down Expand Up @@ -59,7 +57,7 @@ public function onMessageFailed(WorkerMessageFailedEvent $event)
'class' => \get_class($message),
];

$shouldRetry = $retryStrategy && $this->shouldRetry($throwable, $envelope, $retryStrategy);
$shouldRetry = $retryStrategy && $retryStrategy->isRetryable($envelope, $throwable);
Comment on lines -62 to +60
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be reverted.
You moved the logic about Recoverability from SendFailedMessageForRetryListener to the MultiplierRetryStrategy but here, the retry retryStrategy is an RetryStrategyInterface. If people inject another instance, the behavior will not be preserved.


$retryCount = RedeliveryStamp::getRetryCountFromEnvelope($envelope);
if ($shouldRetry) {
Expand All @@ -68,9 +66,10 @@ public function onMessageFailed(WorkerMessageFailedEvent $event)
++$retryCount;

$delay = $retryStrategy->getWaitingTime($envelope, $throwable);
$severity = $retryStrategy->getLogSeverity($envelope, $throwable);

if (null !== $this->logger) {
$this->logger->warning('Error thrown while handling message {class}. Sending for retry #{retryCount} using {delay} ms delay. Error: "{error}"', $context + ['retryCount' => $retryCount, 'delay' => $delay, 'error' => $throwable->getMessage(), 'exception' => $throwable]);
$this->logger->log($severity, 'Error thrown while handling message {class}. Sending for retry #{retryCount} using {delay} ms delay. Error: "{error}"', $context + ['retryCount' => $retryCount, 'delay' => $delay, 'error' => $throwable->getMessage(), 'exception' => $throwable]);
}

// add the delay and retry stamp info
Expand All @@ -84,7 +83,11 @@ public function onMessageFailed(WorkerMessageFailedEvent $event)
}
} else {
if (null !== $this->logger) {
$this->logger->critical('Error thrown while handling message {class}. Removing from transport after {retryCount} retries. Error: "{error}"', $context + ['retryCount' => $retryCount, 'error' => $throwable->getMessage(), 'exception' => $throwable]);
$severity = (null !== $retryStrategy)
? $retryStrategy->getLogSeverity($envelope, $throwable)
: LogLevel::CRITICAL;

$this->logger->log($severity, 'Error thrown while handling message {class}. Removing from transport after {retryCount} retries. Error: "{error}"', $context + ['retryCount' => $retryCount, 'error' => $throwable->getMessage(), 'exception' => $throwable]);
}
}
}
Expand Down Expand Up @@ -121,38 +124,6 @@ public static function getSubscribedEvents(): array
];
}

private function shouldRetry(\Throwable $e, Envelope $envelope, RetryStrategyInterface $retryStrategy): bool
{
if ($e instanceof RecoverableExceptionInterface) {
return true;
}

// if one or more nested Exceptions is an instance of RecoverableExceptionInterface we should retry
// if ALL nested Exceptions are an instance of UnrecoverableExceptionInterface we should not retry
if ($e instanceof HandlerFailedException) {
$shouldNotRetry = true;
foreach ($e->getNestedExceptions() as $nestedException) {
if ($nestedException instanceof RecoverableExceptionInterface) {
return true;
}

if (!$nestedException instanceof UnrecoverableExceptionInterface) {
$shouldNotRetry = false;
break;
}
}
if ($shouldNotRetry) {
return false;
}
}

if ($e instanceof UnrecoverableExceptionInterface) {
return false;
}

return $retryStrategy->isRetryable($envelope, $e);
}

private function getRetryStrategyForTransport(string $alias): ?RetryStrategyInterface
{
if ($this->retryStrategyLocator->has($alias)) {
Expand Down
32 changes: 32 additions & 0 deletions src/Symfony/Component/Messenger/Exception/HandlerFailedException.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,4 +64,36 @@ function ($exception) use ($exceptionClassName) {
)
);
}

/**
* Determine if failure was unrecoverable.
*
* If ALL nested Exceptions are an instance of UnrecoverableExceptionInterface the failure is unrecoverable
*/
public function isUnrecoverable(): bool
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this business logic should belong to this HandlerFailedException class.

I suggest using the existing getNestedExceptionOfClass instead.

{
foreach ($this->exceptions as $nestedException) {
if (!$nestedException instanceof UnrecoverableExceptionInterface) {
return false;
}
}

return true;
}

/**
* Determine if failure was recoverable.
*
* If one or more nested Exceptions is an instance of RecoverableExceptionInterface the failure is recoverable
*/
public function isRecoverable(): bool
{
foreach ($this->exceptions as $nestedException) {
if ($nestedException instanceof RecoverableExceptionInterface) {
return true;
}
}

return false;
}
}
12 changes: 2 additions & 10 deletions src/Symfony/Component/Messenger/Retry/MultiplierRetryStrategy.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@
*/
class MultiplierRetryStrategy implements RetryStrategyInterface
{
use RetryRecoveryBehaviorTrait;

private int $maxRetries;
private int $delayMilliseconds;
private float $multiplier;
Expand Down Expand Up @@ -63,16 +65,6 @@ public function __construct(int $maxRetries = 3, int $delayMilliseconds = 1000,
$this->maxDelayMilliseconds = $maxDelayMilliseconds;
}

/**
* @param \Throwable|null $throwable The cause of the failed handling
*/
public function isRetryable(Envelope $message, \Throwable $throwable = null): bool
{
$retries = RedeliveryStamp::getRetryCountFromEnvelope($message);

return $retries < $this->maxRetries;
}

/**
* @param \Throwable|null $throwable The cause of the failed handling
*/
Expand Down
80 changes: 80 additions & 0 deletions src/Symfony/Component/Messenger/Retry/RetryRecoveryBehaviorTrait.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
<?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\Messenger\Retry;

use Psr\Log\LogLevel;
use Symfony\Component\Messenger\Envelope;
use Symfony\Component\Messenger\Exception\HandlerFailedException;
use Symfony\Component\Messenger\Exception\RecoverableExceptionInterface;
use Symfony\Component\Messenger\Exception\UnrecoverableExceptionInterface;
use Symfony\Component\Messenger\Stamp\RedeliveryStamp;

/**
* Trait for retry strategies containing default log severity rules.
*/
trait RetryRecoveryBehaviorTrait
{
/**
* @param \Throwable|null $throwable The cause of the failed handling
*/
public function isRetryable(Envelope $message, \Throwable $throwable = null): bool
{
if (null !== $throwable) {
// A failure is either unrecoverable, recoverable or neutral
if ($this->isUnrecoverable($throwable)) {
return false;
}

if ($this->isRecoverable($throwable)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the isRecoverable should be checked first.
In previous implementation if the exception contains both recoverable and unrecoverable the method shouldRetry returned true

return true;
}
}

$retries = RedeliveryStamp::getRetryCountFromEnvelope($message);

return $retries < $this->maxRetries;
}

/**
* @return string The \Psr\Log\LogLevel log severity
*/
public function getLogSeverity(Envelope $message, \Throwable $throwable = null): string
{
return $this->isRetryable($message, $throwable)
? LogLevel::WARNING
: LogLevel::CRITICAL;
}

/**
* Determine if exception was unrecoverable.
*
* Unrecoverable exceptions should never be retried
*/
private function isUnrecoverable(\Throwable $throwable): bool
{
return ($throwable instanceof HandlerFailedException)
? $throwable->isUnrecoverable()
: $throwable instanceof UnrecoverableExceptionInterface;
}

/**
* Determine if exception was recoverable.
*
* Recoverable exceptions should always be retried
*/
private function isRecoverable(\Throwable $throwable): bool
{
return ($throwable instanceof HandlerFailedException)
? $throwable->isRecoverable()
: $throwable instanceof RecoverableExceptionInterface;
}
}
6 changes: 6 additions & 0 deletions src/Symfony/Component/Messenger/Retry/RetryStrategyInterface.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
* @author Fabien Potencier <fabien@symfony.com>
* @author Grégoire Pineau <lyrixx@lyrixx.info>
* @author Ryan Weaver <ryan@symfonycasts.com>
* @author Joris Steyn <symfony@j0r1s.nl>
*/
interface RetryStrategyInterface
{
Expand All @@ -31,4 +32,9 @@ public function isRetryable(Envelope $message, \Throwable $throwable = null): bo
* @return int The time to delay/wait in milliseconds
*/
public function getWaitingTime(Envelope $message, \Throwable $throwable = null): int;

/**
* @return string The \Psr\Log\LogLevel log severity
*/
public function getLogSeverity(Envelope $message, \Throwable $throwable = null): string;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's a BC break you must provide migration path in lower versions

And, I've not convinced this method should be part of this interface

}
51 changes: 50 additions & 1 deletion ...Symfony/Component/Messenger/Tests/EventListener/SendFailedMessageForRetryListenerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,12 @@

use PHPUnit\Framework\TestCase;
use Psr\Container\ContainerInterface;
use Psr\Log\LoggerInterface;
use Symfony\Component\Messenger\Envelope;
use Symfony\Component\Messenger\Event\WorkerMessageFailedEvent;
use Symfony\Component\Messenger\EventListener\SendFailedMessageForRetryListener;
use Symfony\Component\Messenger\Exception\RecoverableMessageHandlingException;
use Symfony\Component\Messenger\Exception\UnrecoverableMessageHandlingException;
use Symfony\Component\Messenger\Retry\RetryStrategyInterface;
use Symfony\Component\Messenger\Stamp\DelayStamp;
use Symfony\Component\Messenger\Stamp\RedeliveryStamp;
Expand Down Expand Up @@ -63,7 +65,7 @@ public function testRecoverableStrategyCausesRetry()
$senderLocator->expects($this->once())->method('has')->willReturn(true);
$senderLocator->expects($this->once())->method('get')->willReturn($sender);
$retryStategy = $this->createMock(RetryStrategyInterface::class);
$retryStategy->expects($this->never())->method('isRetryable');
$retryStategy->expects($this->once())->method('isRetryable')->willReturn(true);
$retryStategy->expects($this->once())->method('getWaitingTime')->willReturn(1000);
$retryStrategyLocator = $this->createMock(ContainerInterface::class);
$retryStrategyLocator->expects($this->once())->method('has')->willReturn(true);
Expand Down Expand Up @@ -190,4 +192,51 @@ public function testEnvelopeKeepOnlyTheLast10Stamps()

$listener->onMessageFailed($event);
}

public function testUsesRetryStrategyToDetermineLogSeverityForRecoverableExceptions()
{
$sender = $this->createMock(SenderInterface::class);
$sender->expects($this->once())->method('send')->willReturn(new Envelope(new \stdClass(), []));
$senderLocator = $this->createMock(ContainerInterface::class);
$senderLocator->expects($this->once())->method('has')->willReturn(true);
$senderLocator->expects($this->once())->method('get')->willReturn($sender);
$retryStategy = $this->createMock(RetryStrategyInterface::class);
$retryStategy->expects($this->once())->method('isRetryable')->willReturn(true);
$retryStategy->expects($this->once())->method('getLogSeverity')->willReturn('info');
$retryStrategyLocator = $this->createMock(ContainerInterface::class);
$retryStrategyLocator->expects($this->once())->method('has')->willReturn(true);
$retryStrategyLocator->expects($this->once())->method('get')->willReturn($retryStategy);
$logger = $this->createMock(LoggerInterface::class);
$logger->expects($this->once())->method('log')->with('info');

$listener = new SendFailedMessageForRetryListener($senderLocator, $retryStrategyLocator, $logger);

$exception = new RecoverableMessageHandlingException('retry');
$envelope = new Envelope(new \stdClass());
$event = new WorkerMessageFailedEvent($envelope, 'my_receiver', $exception);

$listener->onMessageFailed($event);
}

public function testUsesRetryStrategyToDetermineLogSeverityForUnrecoverableExceptions()
{
$senderLocator = $this->createMock(ContainerInterface::class);
$retryStategy = $this->createMock(RetryStrategyInterface::class);
$retryStategy->expects($this->once())->method('isRetryable')->willReturn(false);
$retryStategy->expects($this->once())->method('getLogSeverity')->willReturn('info');
$retryStrategyLocator = $this->createMock(ContainerInterface::class);
$retryStrategyLocator->expects($this->once())->method('has')->willReturn(true);
$retryStrategyLocator->expects($this->once())->method('get')->willReturn($retryStategy);

$logger = $this->createMock(LoggerInterface::class);
$logger->expects($this->once())->method('log')->with('info');

$listener = new SendFailedMessageForRetryListener($senderLocator, $retryStrategyLocator, $logger);

$exception = new UnrecoverableMessageHandlingException('retry');
$envelope = new Envelope(new \stdClass());
$event = new WorkerMessageFailedEvent($envelope, 'my_receiver', $exception);

$listener->onMessageFailed($event);
}
}
43 changes: 43 additions & 0 deletions src/Symfony/Component/Messenger/Tests/Exception/HandlerFailedExceptionTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@
use PHPUnit\Framework\TestCase;
use Symfony\Component\Messenger\Envelope;
use Symfony\Component\Messenger\Exception\HandlerFailedException;
use Symfony\Component\Messenger\Exception\RecoverableMessageHandlingException;
use Symfony\Component\Messenger\Exception\UnrecoverableMessageHandlingException;
use Symfony\Component\Messenger\Tests\Fixtures\MyOwnChildException;
use Symfony\Component\Messenger\Tests\Fixtures\MyOwnException;

Expand Down Expand Up @@ -57,4 +59,45 @@ public function testThatNestedExceptionClassAreNotFoundIfNotPresent()
$handlerException = new HandlerFailedException($envelope, [$exception]);
$this->assertCount(0, $handlerException->getNestedExceptionOfClass(MyOwnException::class));
}

public function testFailureIsConsideredUnrecoverableWithSingleNestedUnrecoverableExceptions()
{
$envelope = new Envelope(new \stdClass());

$handlerException = new HandlerFailedException($envelope, [new UnrecoverableMessageHandlingException()]);
$this->assertTrue($handlerException->isUnrecoverable());
}

public function testFailureIsConsideredUnrecoverableWithMultipleNestedUnrecoverableExceptions()
{
$envelope = new Envelope(new \stdClass());

$handlerException = new HandlerFailedException($envelope, [new UnrecoverableMessageHandlingException(), new UnrecoverableMessageHandlingException()]);
$this->assertTrue($handlerException->isUnrecoverable());
}

public function testFailureIsNotConsideredUnrecoverableWhenNotAllExceptionsAreUnrecoverable()
{
$envelope = new Envelope(new \stdClass());

$handlerException = new HandlerFailedException($envelope, [new UnrecoverableMessageHandlingException(), new \LogicException()]);
$this->assertFalse($handlerException->isUnrecoverable());
}

public function testFailureIsConsideredRecoverableWithAnyNestedRecoverableExceptions()
{
$envelope = new Envelope(new \stdClass());

$handlerException = new HandlerFailedException($envelope, [new \LogicException(), new RecoverableMessageHandlingException()]);
$this->assertTrue($handlerException->isRecoverable());
}

public function testFailureIsConsideredNeutralWithNoRecoverableOrUnrecoverableExceptions()
{
$envelope = new Envelope(new \stdClass());

$handlerException = new HandlerFailedException($envelope, [new \LogicException()]);
$this->assertFalse($handlerException->isUnrecoverable());
$this->assertFalse($handlerException->isRecoverable());
}
}
Loading