Skip to content

Messages retried through FailedMessagesRetryCommand do not take retry mechanism into account #35449

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.

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
chq81 opened this issue Jan 23, 2020 · 6 comments

Comments

@chq81
Copy link

chq81 commented Jan 23, 2020

Symfony version(s) affected: 4.4.2 upwards and 5.0.*

Description
I noticed that despite having a retry mechanism in place for a failure transport, messages does not get retried if run by the FailureMessagesRetryCommand.
This is due to the fact that the provided maximum number of messages for the StopWorkerOnMessageLimitListener in the command is 1, which results in the listener stopping the worker after one attempted handling of the message, not taking the retry mechanism into account.

How to reproduce

This is the retry mechanism (making use of the doctrine transport):

failure:
    dsn: 'doctrine://database'
    options:
        table_name: failed_messages
        auto_setup: false
    retry_strategy:
        max_retries: 3
        delay: 1000
        multiplier: 10
        max_delay: 0

When consuming a message in the respective handler, just throw an exception.
When running it the first time, it will get retried three times.

Then, run php bin/console messenger:failed:retry -vv.
You will notice it just runs once before the worker is stopped.

Additional context

See the following code fragments:

Symfony\Component\Messenger\Command\FailedMessagesRetryCommand

protected function execute(InputInterface $input, OutputInterface $output)
{
    $this->eventDispatcher->addSubscriber(new StopWorkerOnMessageLimitListener(1));
    ...
}

Symfony\Component\Messenger\EventListener\StopWorkerOnMessageLimitListener

public function onWorkerRunning(WorkerRunningEvent $event): void
{
    if (!$event->isWorkerIdle() && ++$this->receivedMessages >= $this->maximumNumberOfMessages) {
        $this->receivedMessages = 0;
        $event->getWorker()->stop();

        if (null !== $this->logger) {
            $this->logger->info('Worker stopped due to maximum count of {count} messages processed', ['count' => $this->maximumNumberOfMessages]);
        }
    }
}

Symfony\Component\Messenger\Worker

public function run(array $options = []): void
{
       ...

        while (false === $this->shouldStop) {
            $envelopeHandled = false;
            foreach ($this->receivers as $transportName => $receiver) {
                $envelopes = $receiver->get();

                foreach ($envelopes as $envelope) {
                    $envelopeHandled = true;

                    $this->handleMessage($envelope, $receiver, $transportName);
                    $this->dispatchEvent(new WorkerRunningEvent($this, false));

                    if ($this->shouldStop) {  <--- This is where the worker stops despite the retry mechanism.
                        break 2;
                    }
                }

                // after handling a single receiver, quit and start the loop again
                // this should prevent multiple lower priority receivers from
                // blocking too long before the higher priority are checked
                if ($envelopeHandled) {
                    break;
                }
            }

            if (false === $envelopeHandled) {
                $this->dispatchEvent(new WorkerRunningEvent($this, true));

                usleep($options['sleep']);
            }
        }

        $this->dispatchEvent(new WorkerStoppedEvent($this));
    }

@chq81
Copy link
Author

chq81 commented Jan 24, 2020

I found this issue which explains the reason why that is: #33685

I still would like to ask if this is really wanted behavior?
That even if a retry mechanism is in place for failure transports, it only runs it once and if failing, sends the message back into the failure transport.
Why do not send the message back to the original transport and let this mechanism handle the message? Is the reason that we do want to skip the original transport in favor of handling it directly?

@Tobion
Copy link
Contributor

Tobion commented Jan 26, 2020

Is the reason that we do want to skip the original transport in favor of handling it directly?

Yes. The original transport could be full. The failure transport is for manual retries. For automatic retries the retry configuration of the orignal transport is already in effect.

@chq81
Copy link
Author

chq81 commented Jan 29, 2020

@Tobion Thanks for clarifying. Would it make sense to add that to the documentation of the failure transport?

@carsonbot
Copy link

Hey, thanks for your report!
There has not been a lot of activity here for a while. Is this bug still relevant? Have you managed to find a workaround?

@carsonbot
Copy link

Could I get a reply or should I close this?

@carsonbot
Copy link

Hey,

I didn't hear anything so I'm going to close it. Feel free to comment if this is still relevant, I can always reopen!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants