[Messenger] Refactoring failure to FailedMessage & allowing for requeue #31397

weaverryan · 2019-05-06T16:22:14Z

Q	A
Branch?	master
Bug fix?	partially
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	none
License	MIT
Doc PR	symfony/symfony-docs#11236

There are a few things going on:

Purely because I think it's a better implementation, I've created a new FailedMessage class and the failed messages go inside of this. This message is then routed to the failure transport.
This adds the ability to "requeue" messages instead of retrying them immediately, based on feedback from Twitter.
This should fix an undiscovered bug: if you "retry" and use [Messenger] Allows to register handlers on a specific transport #30958, previously, retrying would not execute the correct handlers because it would appear the message was being received from the "failure" transport, not the original transport, which has configuration to execute specific handlers.

Cheers!

weaverryan

Much of this PR is just a revert of part of #30970. I've highlighted those to help review.

weaverryan · 2019-05-06T17:23:10Z

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

@@ -1768,29 +1788,11 @@ private function registerMessengerConfiguration(array $config, ContainerBuilder

        $container->getDefinition('messenger.senders_locator')
            ->replaceArgument(0, $messageToSendersMapping)
-            ->replaceArgument(1, ServiceLocatorTagPass::register($container, $senderReferences))
-            ->replaceArgument(2, $messagesToSendAndHandle)
+            ->replaceArgument(1, $messagesToSendAndHandle)


All of the logic from $senders = [] is a revert of #30970

weaverryan · 2019-05-06T17:24:25Z

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

+            if (!isset($senderAliases[$failureTransport])) {
+                $senderAliases[$failureTransport] = $failureTransport;
+            }
+        }


This section was mostly just moved from below. The $messagesToSendersMapping is the new part: it adds routing for the FailedMessage class to whatever the failure transport is.

weaverryan · 2019-05-06T17:25:05Z

src/Symfony/Bundle/FrameworkBundle/Resources/config/console.xml

-
-            <tag name="console.command" command="messenger:failed:remove" />
-        </service>
-


Moved into a new file so we can conditionally import that file (instead importing, then removing if the failure transport is disabled).

weaverryan · 2019-05-06T17:25:33Z

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

@@ -9,8 +9,7 @@

        <!-- Asynchronous -->
        <service id="messenger.senders_locator" class="Symfony\Component\Messenger\Transport\Sender\SendersLocator">
-            <argument type="collection" /> <!-- Per message senders map -->
-            <argument /> <!-- senders locator -->
+            <argument type="collection" /> <!-- Per message sender iterators -->
            <argument type="collection" /> <!-- Messages to send and handle -->


Revert of #30970

weaverryan · 2019-05-06T17:25:45Z

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

-        $this->assertEquals(new Reference('audit'), $sendersLocator->getArgument(0)['audit']->getValues()[0]);
+            'amqp' => new Reference('messenger.transport.amqp'),
+            'audit' => new Reference('audit'),
+        ], $sendersMapping[DummyMessage::class]->getValues());


Revert of #30970

weaverryan · 2019-05-06T17:34:34Z

src/Symfony/Component/Messenger/Stamp/RedeliveryStamp.php

+        }
+
+        return false;
+    }


Revert of #30970

weaverryan · 2019-05-06T17:35:24Z

src/Symfony/Component/Messenger/Tests/Middleware/SendMessageMiddlewareTest.php

-            });
-
-        return new SendersLocator($sendersMap, $container, $sendAndHandle);
-    }


Whole class is a revert of #30970

weaverryan · 2019-05-06T17:35:43Z

src/Symfony/Component/Messenger/Tests/Stamp/RedeliveryStampTest.php

+        $this->assertTrue($stampToRedeliverToSender1->shouldRedeliverToSender('App\Sender1', null));
+        $this->assertFalse($stampToRedeliverToSender1->shouldRedeliverToSender('App\Sender2', null));
+    }
+
    public function testGetters()
    {
        $stamp = new RedeliveryStamp(10, 'sender_alias');


Revert of #30970

weaverryan · 2019-05-06T17:35:50Z

src/Symfony/Component/Messenger/Tests/Transport/Sender/SendersLocatorTest.php

-            });
-
-        return $container;
+        $this->assertSame(['dummy' => $sender], iterator_to_array($locator->getSenders(new Envelope(new DummyMessage('a')))));


Revert of #30970

weaverryan · 2019-05-06T17:35:57Z

src/Symfony/Component/Messenger/Transport/Sender/SendersLocator.php

-        }
-
-        throw new UnknownSenderException(sprintf('Unknown sender alias "%s".', $alias));
-    }


Revert of #30970

weaverryan · 2019-05-06T18:56:29Z

src/Symfony/Component/Messenger/Tests/Failure/FailureIntegrationTest.php

+use Symfony\Component\Messenger\Transport\Sender\SendersLocator;
+use Symfony\Component\Messenger\Worker;
+
+class FailureIntegrationTest extends TestCase


This test is a bit crazy. There are various layers that interact, and this proves the work together:

The ability to send a message to multiple transports but some handlers are only called on some transports

The failure transport and the requeuing of messages

The failure transport and retrying messages (instead of requeuing) and making sure the correct handlers are still called.

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

Tobion · 2019-05-07T15:53:27Z

I like the new implementation. But now that FailedMessage are routed using the standard config, does it not mean that the FailedMessage will also be routed to transports configured by the user using the wildcard, e.g. * => amqp. So the FailedMessage would go to the failure transport as well as amqp. Which will probably fail again and then send the message to both transports, creating an endless loop.

src/Symfony/Component/Messenger/Failure/FailedMessageHandler.php

sroze

As discussed on Slack, I think this approach as multiple downsides:

It doesn't work with * as @Tobion spotted well
It forces the failure transport to only use the PHP serialize serializer (because Symfony Serializer couldn't know how to deserialize the FailedMessage message; especially its inner Envelope)
It forces the entirety of the "system" to only have one "failure transport"

Also, all of this change is trigged by the point 2) of the pull-request description, which, IMHO, could be simply achieved by dispatching the message again (without the ReceivedStamp). I agree that we need to look at 3) which might be a bug though.

sroze · 2019-05-07T18:54:34Z

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

@@ -1745,19 +1743,41 @@ private function registerMessengerConfiguration(array $config, ContainerBuilder

        $messageToSendersMapping = [];
        $messagesToSendAndHandle = [];
+        if ($config['failure_transport']) {
+            $failureTransport = $config['failure_transport'];


This line could be in the if :)

weaverryan · 2019-05-07T20:01:50Z

After talking with Sam & Nicolas (and @Tobion's feedback), I'm closing this PR. Most of this code was a refactoring, which didn't contain enough objective improvements over the previous implementation.

We can propose requeue for 4.4, as it's a new feature.

And for:

This should fix an undiscovered bug: if you "retry" and use [Messenger] Allows to register handlers on a specific transport #30958, previously, retrying would not execute the correct handlers because it would appear the message was being received from the "failure" transport, not the original transport, which has configuration to execute specific handlers.

I'll attempt to fix this independently in another PR.

carsonbot added Status: Needs Review Messenger Feature labels May 6, 2019

weaverryan force-pushed the failure-transport-message-requeue branch from 9bd0543 to 86975f5 Compare May 6, 2019 16:28

weaverryan added this to the 4.3 milestone May 6, 2019

weaverryan mentioned this pull request May 6, 2019

[Messenger][4.3] Tracker for changes symfony/symfony-docs#11236

Closed

25 tasks

weaverryan force-pushed the failure-transport-message-requeue branch from 86975f5 to 4c83e1c Compare May 6, 2019 17:21

weaverryan requested a review from sroze as a code owner May 6, 2019 17:21

weaverryan force-pushed the failure-transport-message-requeue branch from 4c83e1c to 6b19228 Compare May 6, 2019 17:22

weaverryan commented May 6, 2019

View reviewed changes

weaverryan force-pushed the failure-transport-message-requeue branch 4 times, most recently from 8e62086 to d4f483f Compare May 6, 2019 18:54

weaverryan commented May 6, 2019

View reviewed changes

Tobion reviewed May 7, 2019

View reviewed changes

src/Symfony/Component/Messenger/Failure/SendFailedMessageToFailureTransportListener.php Show resolved Hide resolved

Tobion reviewed May 7, 2019

View reviewed changes

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

weaverryan force-pushed the failure-transport-message-requeue branch 2 times, most recently from 6020703 to 1b0b49d Compare May 7, 2019 15:26

Tobion reviewed May 7, 2019

View reviewed changes

src/Symfony/Component/Messenger/Failure/FailedMessageHandler.php Outdated Show resolved Hide resolved

weaverryan force-pushed the failure-transport-message-requeue branch from 1b0b49d to c776abd Compare May 7, 2019 19:11

[Messenger] Refactoring failure to FailedMessage & allowing for requeue

3ab3a85

weaverryan force-pushed the failure-transport-message-requeue branch from c776abd to 3ab3a85 Compare May 7, 2019 19:12

carsonbot removed the Status: Needs Review label May 7, 2019

sroze suggested changes May 7, 2019

View reviewed changes

carsonbot added the Status: Needs Work label May 7, 2019

weaverryan closed this May 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Messenger] Refactoring failure to FailedMessage & allowing for requeue #31397

[Messenger] Refactoring failure to FailedMessage & allowing for requeue #31397

weaverryan commented May 6, 2019 •

edited

Loading

weaverryan left a comment

weaverryan May 6, 2019

weaverryan May 6, 2019

weaverryan May 6, 2019

weaverryan May 6, 2019

weaverryan May 6, 2019

weaverryan May 6, 2019

weaverryan May 6, 2019

weaverryan May 6, 2019

weaverryan May 6, 2019

weaverryan May 6, 2019

weaverryan May 6, 2019

Tobion commented May 7, 2019 •

edited

Loading

sroze left a comment

sroze May 7, 2019

weaverryan commented May 7, 2019


		<tag name="console.command" command="messenger:failed:remove" />
		</service>

[Messenger] Refactoring failure to FailedMessage & allowing for requeue #31397

[Messenger] Refactoring failure to FailedMessage & allowing for requeue #31397

Conversation

weaverryan commented May 6, 2019 • edited Loading

weaverryan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tobion commented May 7, 2019 • edited Loading

sroze left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weaverryan commented May 7, 2019

weaverryan commented May 6, 2019 •

edited

Loading

Tobion commented May 7, 2019 •

edited

Loading