Skip to content

Fixing a bug where messenger:consume could send message to wrong bus #30652

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

Fixing a bug where messenger:consume could send message to wrong bus #30652

wants to merge 2 commits into from

Conversation

weaverryan
Copy link
Member

@weaverryan weaverryan commented Mar 22, 2019 •
edited
Loading

Q A
Branch? master
Bug fix? yes
New feature? arguably, yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #30631
License MIT
Doc PR Not needed

This fixes #30631, where you can run messener:consume and accidentally sent received messages into the wrong bus.

The fix (done via middleware) is to attach a "bus name" to the Envelope and use it when the message is received to find that bus.

@weaverryan weaverryan requested a review from sroze as a code owner March 22, 2019 18:30
@weaverryan weaverryan mentioned this pull request Mar 22, 2019
Closed
@weaverryan weaverryan force-pushed the bus-name-on-envelope branch from 64d3186 to ab3d019 Compare March 22, 2019 18:31
@weaverryan weaverryan changed the title Fixing a bug where a transport could receive a message and dispatch i… Fixing a bug where messenger:consume could send message to wrong bus Mar 22, 2019
@chalasr chalasr added this to the next milestone Mar 22, 2019
@weaverryan weaverryan force-pushed the bus-name-on-envelope branch 4 times, most recently from 41069af to a4f9bf3 Compare March 22, 2019 19:17
@weaverryan
Copy link
Member Author

This is ready to go!

@weaverryan weaverryan force-pushed the bus-name-on-envelope branch from a4f9bf3 to 8874733 Compare March 22, 2019 19:33
sroze
sroze approved these changes Mar 23, 2019
View reviewed changes
Copy link
Contributor

@sroze sroze left a comment

Choose a reason for hiding this comment

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

Great, indeed. I was aiming to write such a PR with me (but this small change) 👌

chalasr
chalasr approved these changes Mar 23, 2019
View reviewed changes
Copy link
Member

@chalasr chalasr left a comment

Choose a reason for hiding this comment

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

👍

@weaverryan weaverryan force-pushed the bus-name-on-envelope branch from 26a804a to a94ce84 Compare March 23, 2019 14:21
@sroze sroze force-pushed the bus-name-on-envelope branch from a94ce84 to ef077cf Compare March 23, 2019 14:30
@fabpot
Copy link
Member

fabpot commented Mar 23, 2019

Thank you @weaverryan.

fabpot added a commit that referenced this pull request Mar 23, 2019
@fabpot
feature #30652 Fixing a bug where messenger:consume could send messag…
e512b7e 
…e to wrong bus (weaverryan)

This PR was merged into the 4.3-dev branch.

Discussion
----------

Fixing a bug where messenger:consume could send message to wrong bus

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | yes
| New feature?  | arguably, yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #30631
| License       | MIT
| Doc PR        | Not needed

This fixes #30631, where you can run `messener:consume` and accidentally sent received messages into the wrong bus.

The fix (done via middleware) is to attach a "bus name" to the `Envelope` and use it when the message is received to find that bus.

Commits
-------

ef077cf Fixing a bug where a transport could receive a message and dispatch it to a different bus
@sroze
Copy link
Contributor

sroze commented Mar 23, 2019

Merged by e512b7e but GitHub couldn't recognise it because I pushed another commit in this branch in the meantime, closing.

@sroze sroze closed this Mar 23, 2019
@weaverryan weaverryan deleted the bus-name-on-envelope branch March 23, 2019 18:22
@raress96
Copy link

Just a thought on this, I think it is a BC Break because if you have an application running SF 4.2 which has published a message that will then be consumed by an application running SF 4.3, you will get the error:
Envelope does not contain a BusNameStamp.

Hence I think it is a BC break. Moreover, if for example, some messages exist in a RabbitMQ queue and you updated your app to SF 4.3, then those messages can no longer be processed because they do not have this stamp. I am proposing that if the stamp is not present then the default bus to be used to handle them.

@sroze
Copy link
Contributor

sroze commented Apr 22, 2019

I am proposing that if the stamp is not present then the default bus to be used to handle them.

@raresserban The component being experimental, we allow BC breaks. Though, I agree with you, it doesn't cost much effort to do so. Can you submit a pull request? Thank you!

@weaverryan weaverryan mentioned this pull request Apr 22, 2019
Closed
@weaverryan
Copy link
Member Author

Yes, I agree. I've been trying to "keep in mind" (at least) the migration path for messages that are inside a queue at the moment of upgrading.

See #31200

@nicolas-grekas nicolas-grekas modified the milestones: next, 4.3 Apr 30, 2019
@fabpot fabpot mentioned this pull request May 9, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sroze sroze sroze approved these changes

@chalasr chalasr chalasr requested changes

@fabpot fabpot fabpot approved these changes

Assignees
No one assigned
Labels
Bug Messenger Status: Reviewed
Projects
None yet
Milestone
4.3
Development

Successfully merging this pull request may close these issues.

[Messenger] messenger:consume - bus should be determined automatically
7 participants
@weaverryan @fabpot @sroze @raress96 @chalasr @nicolas-grekas @carsonbot