fix: MQTT messages are forwarded twice to subscribers with overlapping subscriptions#3789
Conversation
Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>
Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>
| for client in matches { | ||
| for client in HashSet::<ClientId>::from_iter(matches) { |
There was a problem hiding this comment.
This fixes the issue, but might not be the best way. Can this de-duplication be done internally when collecting the subscribers from the trie?
There was a problem hiding this comment.
I believe we could deduplicate in the trie logic, but I don't think we would gain anything over this solution by doing that.
| // a more convincing example would be to subscribe to "+/b" | ||
| // but in that case rumqttd sends the message twice | ||
| // something we can do little about | ||
| subscribe: ["a/b".into()].into(), |
There was a problem hiding this comment.
Adding a subscription to +/b leads to 4 messages being delivered to the client.
Robot Results
|
There was a problem hiding this comment.
The fix looks fine and AFAIK, this topic overlap issue was highlighted as a known limitation when dynamic subscription was implemented. So, I doubt if anything else can be done. So, I'll let @jarhodes314 approve the PR.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! 🚀 New features to boost your workflow:
|
When an actor subscribes to overlapping topics, e.g.
a/+and+/b, then all messages received on the overlapping topics (in that casea/b) are forwarded twice to the actor, and even four times with some MQTT brokers (rumqttc and nanomq).tedge_mqtt_extlayer should not add its own set of duplicates.tedge flowswhere the subscriptions are independently configured for each flow.Proposed changes
tedge_mqtt_extremoves duplicates in the list of clients for a messageTypes of changes
Paste Link to the issue
Checklist
just prepare-devonce)just formatas mentioned in CODING_GUIDELINESjust checkas mentioned in CODING_GUIDELINESFurther comments