Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Revert SyncTransport simplification and fix properly #34155

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

Merged
merged 2 commits into from
Oct 31, 2019

Conversation

weaverryan
Copy link
Member

Q A
Branch? 4.4
Bug fix? yes
New feature? no
Deprecations? no
Tickets Fix #34115 (and also related to #34066)
License MIT
Doc PR Not needed

In #34069, I made SyncTransport simpler by removing that transport class and making the whole things a config trick. I felt GREAT about that solution... until i realized two big problems:

  1. It kills using env vars for sync:// because we read the config values at build time - [Messenger][BC-break] Sync transport can no longer be configured by env variable #34115 - that could probably be fixed by adding a factory, but then there is also the next problem

  2. If someone routed a message to [async, sync] (weird, but allowed), my [Messenger] Removing "sync" transport and replacing it with config trick #34069 config solution basically maps this internally to [async], which actually causes the message to not be handled immediately. Basically, my solution only worked if you route a message ONLY to one sync transport, but fails if you route to multiple transports.

So... this fixes things in a less-cool, but sensible way:

A) The first commit reverts #34069 exactly
B) The second commit solves the issue that we need to know if a message is being handled in a "worker" context or not, so middleware can decide if they should reset things before/after handling things. Previously we were using ReceivedStamp to know this. But because SyncTransport also "receives" the message and adds this stamp, it's not enough. To fix this, I added a new ConsumedByWorkerStamp that clearly means: "This message is being handled by a worker" (and so, you might want to "reset" some things before/after handling).

Thanks!

Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

Makes sense, although it's strange to have two ways to send things "sync".
Maybe the other way to be sync is the one that should be removed?
Anyway, barely related :)

@@ -132,7 +133,7 @@ private function handleMessage(Envelope $envelope, ReceiverInterface $receiver,
];

try {
$envelope = $this->bus->dispatch($envelope->with(new ReceivedStamp($transportName)));
$envelope = $this->bus->dispatch($envelope->with(new ReceivedStamp($transportName), new ConsumedByWorkerStamp()));
Copy link
Member

Choose a reason for hiding this comment

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

Is there anything the worker might want to pass to the chain? E.g. the $output of the command? I'm just wondering :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Even though this stamp won't be "sent" on redelivery, it feels odd to pass what's really a "service" into a Stamp. Better might be (if someone needs this) to add it to one of the worker events we dispatch.

Copy link
Member

@nicolas-grekas nicolas-grekas Oct 28, 2019

Choose a reason for hiding this comment

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

I see no issues on my side doing so, the day we need to.

@weaverryan
Copy link
Member Author

Maybe the other way to be sync is the one that should be removed?
Anyway, barely related :)

It is something we could think about - by adding some sort of a "fallback" transport - i.e. the transport to use if no other transports are matched. I think it's safe to do this later - 5.1, etc if we want to.

@weaverryan weaverryan force-pushed the fix-sync-transport-again branch from 9411cd3 to 7b8d1ea Compare October 28, 2019 16:52
@weaverryan weaverryan force-pushed the fix-sync-transport-again branch from 7b8d1ea to 01a9fef Compare October 28, 2019 17:18
@weaverryan
Copy link
Member Author

Only failure is because symfony/doctrine-bridge:dev-master tests are looking for the new ConsumedByWorkerStamp inside symfony/messenger:dev-master, but it won't be there until this is merged up to master. But if this is fixable by a composer.json constraint, let me know.

@Tobion
Copy link
Contributor

Tobion commented Oct 31, 2019

Thank you @weaverryan.

@Tobion Tobion closed this in cf10c02 Oct 31, 2019
@Tobion Tobion merged commit 01a9fef into symfony:4.4 Oct 31, 2019
@weaverryan weaverryan deleted the fix-sync-transport-again branch October 31, 2019 21:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Messenger][BC-break] Sync transport can no longer be configured by env variable
5 participants
Morty Proxy This is a proxified and sanitized view of the page, visit original site.