[Messenger] multiple failure transports support #34979

monteiro · 2019-12-14T08:55:02Z

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
Deprecations?	no
Tickets	Fix #34911
License	MIT
Doc PR	symfony/symfony-docs#13489

Strategy applied

Pass a map of transports and failed transports to the SendFailedMessageToFailureTransportListener. This way we re-use the same listener.
Local failed transport has more priority than a global failed transport defined.

Configuration example

# config/packages/messenger.yaml
framework:
    # no need to set failed transport globally if I want a specific behaviour per transport.
    failure_transport: failed # all transports have this failed transport
    messenger:
        transports:
            failed: 'doctrine://default?queue_name=failed'
            failed_important: 'doctrine://default?queue_name=failed_important'
            async:
                dsn: '%env(MESSENGER_TRANSPORT_DSN)%'
                failure_transport: failed # takes precedence over the global defined "failed_transport"
                retry_strategy:
                    max_retries: 3
                    delay: 1000
                    multiplier: 2
            async_no_failure_transport: # it will use the global defined transport if no one is defined.
                dsn: '%env(MESSENGER_TRANSPORT_DSN)%'
                retry_strategy:
                    max_retries: 3
                    delay: 1000
                    multiplier: 2
            async_send_specific_failure_queue:
                dsn: '%env(MESSENGER_TRANSPORT_DSN)%'
                failed_transport: failed_important # takes precedence over the global defined "failed_transport"
                retry_strategy:
                    max_retries: 3
                    delay: 1000
                    multiplier: 2

You can test this feature easily on a demo project. Just follow the README.

More information on issue #34911

What needs to be done so this can be merged:

validate strategy
update src/**/CHANGELOG.md files
update tests to cover all cases
create doc PR

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php

src/Symfony/Component/Messenger/EventListener/SendFailedMessageToFailureTransportListener.php

monteiro · 2020-01-20T12:18:31Z

@sroze thanks a lot for the review.

monteiro · 2020-03-25T15:24:59Z

Can someone help me with the review?

I was very careful in order to not break any command or any class by extending some classes with more constructor parameters or adding more options to commands (the failed commands).

I probably need more tests on the FrameworkBundle (extension part).

giovannialbero1992 · 2020-07-09T10:45:34Z

What is missing in this PR? Only Changelog updates?

monteiro · 2020-09-20T15:58:51Z

@fabpot @weaverryan This PR is ready to be reviewed.

I created a symfony project, so anyone can test with my latest changes.

src/Symfony/Component/Messenger/EventListener/SendFailedMessageToFailureTransportListener.php

weaverryan · 2020-09-15T13:01:14Z

src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/FrameworkExtensionTest.php

+            $this->assertSame('messenger.transport.'.$expectedFailureTransportsMapping[$transportName], (string) $ref, sprintf('The transport "%s" does not have the expected failed transport reference', $transportName));
+        }
+
+        $failedMessageTransportListenerReference =


Nit pick - this is really a Definition, not a Reference

weaverryan · 2020-09-29T10:29:51Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php

            $container->getDefinition('console.command.messenger_failed_messages_show')
-                ->replaceArgument(0, $config['failure_transport']);
+                ->replaceArgument(0, $globalFailureReceiver)
+                ->replaceArgument(1, $container->getDefinition($failureTransportsServiceLocatorId));


these should be references, right? We don't normally set a Definition directly on an argument. And should it use $failureTransportsServiceLocatorId or $failureTransportsServiceLocator - I'm getting lost (this stuff confuses me) in which is which and what should be used.

weaverryan · 2020-09-29T10:33:38Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php

-                ->replaceArgument(0, $config['failure_transport']);
+                ->replaceArgument(0, $globalFailureReceiver)
+                ->replaceArgument(1, $senderReferences[$config['failure_transport']] ?? null)
+                ->replaceArgument(5, $container->getDefinition($failureTransportsServiceLocator));


Same problem here (ish) as below - we should pass a Reference (which $failureTransportsServiceLocator is), not a Definition.

weaverryan · 2020-09-29T10:40:37Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php

            $container->removeDefinition('console.command.messenger_failed_messages_retry');
            $container->removeDefinition('console.command.messenger_failed_messages_show');
            $container->removeDefinition('console.command.messenger_failed_messages_remove');
+            $container->removeDefinition('messenger.failure.send_failed_message_to_failure_transport_listener');


we could revert this change to lessen the diff

weaverryan · 2020-09-29T10:42:37Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php

+            if ($transport['failure_transport']) {
+                if (!isset($config['transports'][$transport['failure_transport']])) {
+                    throw new LogicException(sprintf('Invalid Messenger configuration: the failure transport "%s" is not a valid transport or service id.', $transport['failure_transport']));
+                }


Should we be checking for !isset($senderReferences[$transport['failure_transport']]) here instead? That would be consistent with the above code. It's more confusing, but there is the edge case that the failure_transport is not a registered transport, but just a "sender service id"

weaverryan · 2020-09-29T10:53:53Z

src/Symfony/Bundle/FrameworkBundle/Resources/config/messenger.php

+                abstract_arg('failed transports map by transport name'),
+            ])
+            ->tag('container.service_locator')
+


If I'm correct in FrameworkExtension, these won't be needed.

src/Symfony/Component/Messenger/Command/FailedMessagesRetryCommand.php

weaverryan · 2020-09-29T11:11:21Z

src/Symfony/Component/Messenger/DependencyInjection/MessengerPass.php

-                $definition = $container->getDefinition($failedCommandId);
-                $definition->replaceArgument(1, $receiverMapping[$definition->getArgument(0)]);
-            }
-        }


Hmm, wait a second. This is subtle. But currently, in FrameworkExtension, you are populating arg 0 with $senderReferences[$config['failure_transport']]. But before, we were populating them with receiver references. Even for me, this makes my head spin a bit :P. You could technically have a failure transport "receiver" service but no "sender" service... though this is (at the very least) an edge case. Usually a sender & receiver are a "pair" because you've defined them as a transport. But this doesn't need to be the case.

The situation would be this: I create a ReceiverInterface service and tag it with messenger.receiver and alias my_failure_receiver. But, I do not actually register this as a transport. Then, at the command line, I use messenger:failure:show --failure-transport=my_failure_receiver. That is technically legal. And it's even possible (though unlikely) to set up this kind of situation if you're using AMQP (e.g. you actually set up a true failure_transport called failure1, but then with custom binding & routing rules, those messages ultimately end up in a queue connected with the custom my_failure_receiver receiver.

So.... that's why this code was originally here and I think it probably still needs to be. I don't think that's a problem, other than some of the new code in FrameworkExtension should actually live here.

fabpot · 2020-10-07T15:54:29Z

We've just moved away from master as the main branch to use 5.x instead. Unfortunately, I cannot reopen the PR and change the target branch to 5.x. Can you open a new PR referencing this one to not loose the discussion? Thank you for your understanding and for your help.

monteiro · 2020-10-07T16:48:33Z

opened new PR #38468 with 5.x as base

luigi370 · 2022-06-15T14:36:58Z

This is just merged into symfony 5 right? is there any change to make it work on symfony 4.4 without updating the framework? thanks!

stof · 2022-06-15T14:42:32Z

updating the framework...
Old versions of Symfony don't get the new features of newer versions (that would defeat the purpose of versioning)

monteiro requested a review from sroze as a code owner December 14, 2019 08:55

carsonbot added Status: Needs Review Feature labels Dec 14, 2019

monteiro commented Dec 14, 2019

View reviewed changes

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php Outdated Show resolved Hide resolved

nicolas-grekas added this to the next milestone Dec 15, 2019

monteiro force-pushed the messenger-multiple-failed-transports branch from 9a7eb40 to 099a646 Compare December 27, 2019 13:51

monteiro requested review from dunglas, lyrixx and xabbuh as code owners December 27, 2019 13:51

monteiro changed the base branch from 4.4 to master December 27, 2019 13:51

monteiro force-pushed the messenger-multiple-failed-transports branch from 93c5188 to f995ffa Compare December 27, 2019 17:48

monteiro changed the title ~~messenger: multiple failed transports support~~ [Messenger] multiple failed transports support Jan 2, 2020

sroze suggested changes Jan 19, 2020

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Needs Review labels Jan 19, 2020

sroze added the Messenger label Jan 19, 2020

monteiro force-pushed the messenger-multiple-failed-transports branch from 7ae1176 to b4f7e61 Compare January 21, 2020 12:51

maks-rafalko mentioned this pull request Jan 31, 2020

[Messenger] Send message to Failure Transport if async Sender fails #35521

Closed

This was referenced Feb 9, 2020

[Messenger] Reproduce messages to different transport for retrying #34235

Closed

[Messenger] Add middleware to stop all workers on exception #35481

Closed

monteiro force-pushed the messenger-multiple-failed-transports branch from 22493ed to 5f3e52f Compare March 8, 2020 11:40

monteiro force-pushed the messenger-multiple-failed-transports branch from 5f3e52f to 98eaef7 Compare March 25, 2020 12:31

monteiro requested a review from sroze March 25, 2020 15:25

carsonbot added Status: Needs Review and removed Status: Needs Work labels Mar 25, 2020

monteiro mentioned this pull request Apr 4, 2020

messenger: add multiple transports support documentation symfony/symfony-docs#13489

Closed

Nyholm mentioned this pull request May 28, 2020

New DSN component #36999

Closed

3 tasks

fix

7b4481a

monteiro force-pushed the messenger-multiple-failed-transports branch from 252c1ef to 7b4481a Compare September 20, 2020 15:30

monteiro added 2 commits September 20, 2020 16:46

fix retry command

5b662b5

fix

3e0aeae

monteiro requested a review from weaverryan September 20, 2020 16:01

carsonbot added Status: Needs Review and removed Status: Needs Work labels Sep 20, 2020

monteiro changed the title ~~[Messenger] multiple failed transports support~~ [Messenger] multiple failure transports support Sep 21, 2020

monteiro commented Sep 21, 2020

View reviewed changes

src/Symfony/Component/Messenger/EventListener/SendFailedMessageToFailureTransportListener.php Outdated Show resolved Hide resolved

weaverryan reviewed Sep 29, 2020

View reviewed changes

src/Symfony/Component/Messenger/Command/FailedMessagesRetryCommand.php Outdated Show resolved Hide resolved

weaverryan reviewed Sep 29, 2020

View reviewed changes

monteiro added 4 commits October 1, 2020 17:32

fix SendFailedMessageToFailureTransportListenerTest

5b3eb0d

remove failureTransports last argument of FailedMessagesRetryCommand

27eb470

php cs fix

077060a

wip

353fc62

fabpot closed this Oct 7, 2020

nicolas-grekas added the for 5.x label Oct 7, 2020

nicolas-grekas added for 5.x and removed for 5.x labels Oct 7, 2020

monteiro mentioned this pull request Apr 1, 2021

[Messenger] Multiple failed transports symfony/symfony-docs#15177

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Messenger] multiple failure transports support #34979

[Messenger] multiple failure transports support #34979

monteiro commented Dec 14, 2019 •

edited

Loading

monteiro commented Jan 20, 2020 •

edited

Loading

monteiro commented Mar 25, 2020 •

edited

Loading

giovannialbero1992 commented Jul 9, 2020

monteiro commented Sep 20, 2020 •

edited

Loading

weaverryan Sep 15, 2020

weaverryan Sep 29, 2020

weaverryan Sep 29, 2020

weaverryan Sep 29, 2020

weaverryan Sep 29, 2020

weaverryan Sep 29, 2020

weaverryan Sep 29, 2020

fabpot commented Oct 7, 2020

monteiro commented Oct 7, 2020 •

edited

Loading

luigi370 commented Jun 15, 2022

stof commented Jun 15, 2022

[Messenger] multiple failure transports support #34979

[Messenger] multiple failure transports support #34979

Conversation

monteiro commented Dec 14, 2019 • edited Loading

Strategy applied

Configuration example

What needs to be done so this can be merged:

monteiro commented Jan 20, 2020 • edited Loading

monteiro commented Mar 25, 2020 • edited Loading

giovannialbero1992 commented Jul 9, 2020

monteiro commented Sep 20, 2020 • edited Loading

weaverryan Sep 15, 2020

Choose a reason for hiding this comment

weaverryan Sep 29, 2020

Choose a reason for hiding this comment

weaverryan Sep 29, 2020

Choose a reason for hiding this comment

weaverryan Sep 29, 2020

Choose a reason for hiding this comment

weaverryan Sep 29, 2020

Choose a reason for hiding this comment

weaverryan Sep 29, 2020

Choose a reason for hiding this comment

weaverryan Sep 29, 2020

Choose a reason for hiding this comment

fabpot commented Oct 7, 2020

monteiro commented Oct 7, 2020 • edited Loading

luigi370 commented Jun 15, 2022

stof commented Jun 15, 2022

monteiro commented Dec 14, 2019 •

edited

Loading

monteiro commented Jan 20, 2020 •

edited

Loading

monteiro commented Mar 25, 2020 •

edited

Loading

monteiro commented Sep 20, 2020 •

edited

Loading

monteiro commented Oct 7, 2020 •

edited

Loading