-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
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
Conversation
…uch nicer config trick" This reverts commit 3d4e59a.
There was a problem hiding this 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())); |
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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. |
9411cd3
to
7b8d1ea
Compare
7b8d1ea
to
01a9fef
Compare
Only failure is because |
Thank you @weaverryan. |
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: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 problemIf 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 becauseSyncTransport
also "receives" the message and adds this stamp, it's not enough. To fix this, I added a newConsumedByWorkerStamp
that clearly means: "This message is being handled by a worker" (and so, you might want to "reset" some things before/after handling).Thanks!