Skip to content

[Messenger] Removing "sync" transport and replacing it with config trick #34069

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 1 commit into from
Oct 22, 2019

Conversation

weaverryan
Copy link
Member

Q A
Branch? 4.4
Bug fix? no (but removes some hackiness)
New feature? no
Deprecations? yes (BC break yes)
Tickets Helps #34066
License MIT
Doc PR not needed

I introduced SyncTransport in 4.3 as a way to allow people to, for example, route a message to an async transport and then override that to a sync transport in the dev environment. The implementation was always a bit of a hack, and makes this one transport behave different than others and makes messages routed to this transport behave a bit different than sync messages that are simply not routed at all.

Thanks for an idea from Nicolas, this fixes that. Gone are SyncTransport. Instead, the sync:// transport string is still supported, but it's just a "config" trick: messages routed to transports that are "sync" are simply filtered out of the "senders" in the DI extension. Nice & clean.

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.

sweet, less code !

@@ -4,6 +4,8 @@ CHANGELOG
4.4.0
-----

* [BC BREAK] The `SyncTransport` and `SyncTransportFactory` classes
were removed.
Copy link
Member

Choose a reason for hiding this comment

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

no line split would be good here

throw new LogicException(sprintf('Invalid Messenger routing configuration: the "%s" class is being routed to a sender called "%s". This is not a valid transport or service id.', $message, $sender));
}
}

$messageToSendersMapping[$message] = $messageConfiguration['senders'];
if (0 !== \count($realSenders)) {
Copy link
Member

Choose a reason for hiding this comment

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

if ($realSenders) {

@weaverryan weaverryan force-pushed the better-sync-transport branch from 4aef12a to 3d4e59a Compare October 22, 2019 14:49
@weaverryan weaverryan changed the title [Messenger] Removing "sync" transport and replacing it nice config trick [Messenger] Removing "sync" transport and replacing it with config trick Oct 22, 2019
@fabpot
Copy link
Member

fabpot commented Oct 22, 2019

Thank you @weaverryan.

fabpot added a commit that referenced this pull request Oct 22, 2019
… with config trick (weaverryan)

This PR was merged into the 4.4 branch.

Discussion
----------

[Messenger] Removing "sync" transport and replacing it with config trick

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | no (but removes some hackiness)
| New feature?  | no
| Deprecations? | yes (BC break yes)
| Tickets       | Helps #34066
| License       | MIT
| Doc PR        | not needed

I introduced `SyncTransport` in 4.3 as a way to allow people to, for example, route a message to an `async` transport and then override that to a `sync` transport in the dev environment. The implementation was always a bit of a hack, and makes this one transport behave different than others *and* makes messages routed to this transport behave a *bit* different than sync messages that are simply not routed at all.

Thanks for an idea from Nicolas, this fixes that. Gone are `SyncTransport`. Instead, the `sync://` transport string is still supported, but it's just a "config" trick: messages routed to transports that are "sync" are simply filtered out of the "senders" in the DI extension. Nice & clean.

Commits
-------

3d4e59a [Messenger] Removing "sync" transport and replacing it with much nicer config trick
@fabpot fabpot merged commit 3d4e59a into symfony:4.4 Oct 22, 2019
fabpot added a commit that referenced this pull request Oct 22, 2019
…ng always (weaverryan)

This PR was merged into the 4.4 branch.

Discussion
----------

[Messenger] Fix worker-only Doctrine middleware from running always

| Q             | A
| ------------- | ---
| Branch?       | 4.4 (or 4.3?, this is a bug fix)
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | Fix #32436 Depends on #34069
| License       | MIT
| Doc PR        | not needed

Several Doctrine middleware are only meant to be run in a "worker" context: we want to "ping" the connection, "close" the connection and "clear" the entity manager ONLY when we are receiving messages. Before this PR, it was done always, which causes bad behavior for sync messages (imagine your Doctrine connection being closed in the middle of a controller or see #31334 (comment)).

This fixes that in a pragmatic way: no new system for "worker-only" middleware or anything like that: just make the middleware smart enough to only do their work when a message is being received async.

Commits
-------

290a729 Fixing issue where worker-only middleware were run in all contexts
@weaverryan weaverryan deleted the better-sync-transport branch October 22, 2019 15:54
@Devristo
Copy link
Contributor

This seems to break the case when the transport name is defined using an environment variable:

Works:

framework:
   messenger:
     transports:
       async: "sync://"

Broken when MESSENGER_TRANSPORT=sync://:

framework:
   messenger:
     transports:
       async: "%env(MESSENGER_TRANSPORT)%"

Yields No transport supports the given Messenger DSN "sync://"

@nicolas-grekas nicolas-grekas modified the milestones: next, 4.4 Oct 27, 2019
Tobion added a commit that referenced this pull request Oct 31, 2019
…averryan)

This PR was squashed before being merged into the 4.4 branch (closes #34155).

Discussion
----------

Revert SyncTransport simplification and fix properly

| 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 - #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 #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!

Commits
-------

01a9fef Adding ConsumedByWorkerStamp as way to mark a message in a "worker context"
38f19a9 Revert "[Messenger] Removing "sync" transport and replacing it with much nicer config trick"
This was referenced Nov 12, 2019
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.

6 participants