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

Fix callbacks being replaced on topics with wildcards #388

Merged
merged 4 commits into from Dec 20, 2019
Merged

Fix callbacks being replaced on topics with wildcards #388

merged 4 commits into from Dec 20, 2019

Conversation

v-zhuravlev
Copy link
Contributor

@v-zhuravlev v-zhuravlev commented Dec 5, 2019

I had an issue with router.addRoute described here by fellow MQTT users: #336.
In my case, I have the client with subscriptions to #, topic1, topic2 with different callbacks set for each. Due to e.Value.(*route).match(topic) (https://github.com/eclipse/paho.mqtt.golang/blob/master/router.go#L102) , new route/subscription overrides previous route instead of creating new one and only last callback is working for all routes.

And in my app I expect:
If I publish something to topic1, I expect both callbacks for # and topic1 subscriptions to fire. Not just the last one.

Signed-off-by: v-zhuravlev <zhuravlev.vitaly@gmail.com>
Signed-off-by: v-zhuravlev <zhuravlev.vitaly@gmail.com>
@MattBrittan
Copy link
Contributor

Making this change and not also changing deleteRoute has the potential to be confusing (not that the existing code is not already confusing!). As the pull requestly currently stands calling:

Subscribe("#", xxx)
Subscribe("topic1", yyy)
Unsubscribe("topic1")

Would leave the route for "topic1" in place (Unsubscribe will call deleteRoute("topic1") which will match on "#", remove that route and return). The broker will continue to send all data through but only the callback for topic1 will remain active (definitly not what is intended).

The behaviour of the code in master as it currently stands is just as confusing; the above would result in no routes. However as this is a potentially breaking change to Subscribe then I believe that Unsubscribe/deleteRoute should also be updated to match at the same time.

Go Playground demonstrating this.

Signed-off-by: v-zhuravlev <zhuravlev.vitaly@gmail.com>
Signed-off-by: v-zhuravlev <zhuravlev.vitaly@gmail.com>
@v-zhuravlev
Copy link
Contributor Author

Thanks @MattBrittan for such great comment, changed deleteRoute as well.

@alsm
Copy link
Contributor

alsm commented Dec 20, 2019

Thanks for fixing this up.

@alsm alsm merged commit 529b029 into eclipse:master Dec 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants