Movatterモバイル変換

weaverryan force-pushed thefailure-transport-message-requeue branch from9bd0543 to86975f5Compare

May 6, 2019 16:28

weaverryan added this to the4.3 milestone

weaverryan mentioned this pull request

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

Closed

25 tasks

weaverryan force-pushed thefailure-transport-message-requeue branch from86975f5 to4c83e1cCompare

May 6, 2019 17:21

weaverryan requested a review fromsroze as acode owner

May 6, 2019 17:21

weaverryan force-pushed thefailure-transport-message-requeue branch from4c83e1c to6b19228Compare

May 6, 2019 17:22

weaverryan commented

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

Copy link

MemberAuthor

weaverryan left a comment

Choose a reason for hiding this comment

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

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

		->replaceArgument(0,$messageToSendersMapping)
		->replaceArgument(1, ServiceLocatorTagPass::register($container,$senderReferences))
		->replaceArgument(2,$messagesToSendAndHandle)
		->replaceArgument(1,$messagesToSendAndHandle)

Copy link

MemberAuthor

Choose a reason for hiding this comment

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

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

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

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

Copy link

MemberAuthor

Choose a reason for hiding this comment

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

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

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


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

Copy link

MemberAuthor

Choose a reason for hiding this comment

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

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

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

		<argumenttype="collection" /><!-- Per message senders map-->
		<argument /><!-- senders locator-->
		<argumenttype="collection" /><!-- Per message sender iterators-->
		<argumenttype="collection" /><!-- Messages to send and handle-->

Copy link

MemberAuthor

Choose a reason for hiding this comment

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

Revert of#30970

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

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

Copy link

MemberAuthor

Choose a reason for hiding this comment

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

Revert of#30970

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

		}

		returnfalse;
		}

Copy link

MemberAuthor

Choose a reason for hiding this comment

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

Revert of#30970

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

		});

		returnnewSendersLocator($sendersMap,$container,$sendAndHandle);
		}

Copy link

MemberAuthor

Choose a reason for hiding this comment

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

Whole class is a revert of#30970

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


		publicfunctiontestGetters()
		{
		$stamp =newRedeliveryStamp(10,'sender_alias');

Copy link

MemberAuthor

Choose a reason for hiding this comment

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

Revert of#30970

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

		});

		return$container;
		$this->assertSame(['dummy' =>$sender],iterator_to_array($locator->getSenders(newEnvelope(newDummyMessage('a')))));

Copy link

MemberAuthor

Choose a reason for hiding this comment

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

Revert of#30970

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

		}

		thrownewUnknownSenderException(sprintf('Unknown sender alias "%s".',$alias));
		}

Copy link

MemberAuthor

Choose a reason for hiding this comment

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

Revert of#30970

weaverryan force-pushed thefailure-transport-message-requeue branch 4 times, most recently from8e62086 tod4f483fCompare

May 6, 2019 18:54

weaverryan commented

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

		useSymfony\Component\Messenger\Transport\Sender\SendersLocator;
		useSymfony\Component\Messenger\Worker;

		class FailureIntegrationTestextends TestCase

Copy link

MemberAuthor

Choose a reason for hiding this comment

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

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.

Tobion reviewed

src/Symfony/Component/Messenger/Failure/SendFailedMessageToFailureTransportListener.phpShow resolvedHide resolved

Tobion reviewed

src/Symfony/Component/Messenger/Failure/SendFailedMessageToFailureTransportListener.php OutdatedShow resolvedHide resolved

weaverryan force-pushed thefailure-transport-message-requeue branch 2 times, most recently from6020703 to1b0b49dCompare

May 7, 2019 15:26

Copy link

Contributor

Tobion commentedMay 7, 2019•
edited
Loading

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.

Tobion reviewed

src/Symfony/Component/Messenger/Failure/FailedMessageHandler.php OutdatedShow resolvedHide resolved

weaverryan force-pushed thefailure-transport-message-requeue branch from1b0b49d toc776abdCompare

May 7, 2019 19:11

[Messenger] Refactoring failure to FailedMessage & allowing for requeue

3ab3a85

weaverryan force-pushed thefailure-transport-message-requeue branch fromc776abd to3ab3a85Compare

May 7, 2019 19:12

carsonbot removed the Status: Needs Review label

sroze suggested changes

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

Copy link

Contributor

sroze left a comment

Choose a reason for hiding this comment

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

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 PHPserialize serializer (because Symfony Serializer couldn't know how to deserialize theFailedMessage message; especially its innerEnvelope)
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 theReceivedStamp). I agree that we need to look at 3) which might be a bug though.

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

Copy link

Contributor

srozeMay 7, 2019

Choose a reason for hiding this comment

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

This line could be in theif :)

carsonbot added the Status: Needs Work label

Copy link

MemberAuthor

weaverryan commentedMay 7, 2019

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.

weaverryan closed this