Revert SyncTransport simplification and fix properly #34155
Conversation
…uch nicer config trick" This reverts commit 3d4e59a.
| @@ -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.
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.
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.
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
SyncTransportsimpler 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
ReceivedStampto know this. But becauseSyncTransportalso "receives" the message and adds this stamp, it's not enough. To fix this, I added a newConsumedByWorkerStampthat clearly means: "This message is being handled by a worker" (and so, you might want to "reset" some things before/after handling).Thanks!