Skip to content
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

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

Closed
wants to merge 2 commits into from

Conversation

Projects
None yet
7 participants
@weaverryan
Copy link
Member

commented Mar 22, 2019

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 Mar 22, 2019

@weaverryan weaverryan force-pushed the weaverryan:bus-name-on-envelope branch from 64d3186 to ab3d019 Mar 22, 2019

@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 weaverryan:bus-name-on-envelope branch 4 times, most recently from 41069af to a4f9bf3 Mar 22, 2019

@weaverryan

This comment has been minimized.

Copy link
Member Author

commented Mar 22, 2019

This is ready to go!

@weaverryan weaverryan force-pushed the weaverryan:bus-name-on-envelope branch from a4f9bf3 to 8874733 Mar 22, 2019

@weaverryan weaverryan removed the Deprecation label Mar 22, 2019

@sroze

sroze approved these changes Mar 23, 2019

Copy link
Member

left a comment

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

@chalasr
Copy link
Member

left a comment

👍

@fabpot

fabpot approved these changes Mar 23, 2019

@weaverryan weaverryan force-pushed the weaverryan:bus-name-on-envelope branch from 26a804a to a94ce84 Mar 23, 2019

@sroze sroze force-pushed the weaverryan:bus-name-on-envelope branch from a94ce84 to ef077cf Mar 23, 2019

@fabpot

This comment has been minimized.

Copy link
Member

commented Mar 23, 2019

Thank you @weaverryan.

fabpot added a commit that referenced this pull request Mar 23, 2019

feature #30652 Fixing a bug where messenger:consume could send messag…
…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

This comment has been minimized.

Copy link
Member

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 weaverryan:bus-name-on-envelope branch Mar 23, 2019

@raresserban

This comment has been minimized.

Copy link

commented Apr 22, 2019

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Member Author

commented Apr 22, 2019

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 referenced this pull request May 9, 2019

Merged

Release v4.3.0-BETA1 #31435

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.