search

256dpi / arduino-mqtt

MQTT library for Arduino
MIT License
1.02k stars 236 forks source link

Fixes #197 - by publishing blank retained message to topic before sub #198

Closed BrianSidebotham closed 1 year ago

BrianSidebotham commented 4 years ago

Fixes #197 by adding the publication of a blank retained message immediately before subscribing to the topic.

There's still a possibility of crashing due to a race between the publishing of the retained message and the subscription to the topic. Another source might have published a large retained message between our publish and subscribe commands.

There is commentary to suit. All examples have been updated.

256dpi commented 4 years ago

Thanks for the PR! I'm 100% for making running an example a frictionless experience for the potentially newcomer users of this library. However, after thinking about it a little more I'm not happy with the approach. This is a very specific edge case that now gets a lot of attention by having that big of a comment in the example. Furthermore, the workaround is not water-proof when an "adversary" is actively sending large messages.

My proposal is to add the following comment instead:

// Clear retained messages that may have been published earlier on this topic.
client.publish("/hello", "", 1, true);

This will will fix the "hidden" problem of a lingering large message. In the case that there is an active"adversary" the user will at some point go to shiftr.io/try to look at the namespace and realize that there is another client creating the conflict.

In the future we should fix the problem at the root to make the program not crash on messages that are too big for its buffers and just skip them while logging the error. I'm creating an issue for that now.

BrianSidebotham commented 4 years ago

Great - yes I think that'd be a better long-term goal. All sounds good. I'll adjust the commentary. :+1:

BrianSidebotham commented 4 years ago

@256dpi Have updated the examples again. :+1:

BrianSidebotham commented 3 years ago

Just doing some housekeeping and saw this was still open. Any news?

256dpi commented 3 years ago

Yes, sorry for leaving this stale. I decided to fix the problem at its root. I will update the underlying lwmqtt library to allow dropping packets that are too big. Additionally, I will add a debug callback to arduino-mqtt which we can use to write these events to serial or other outputs.

BrianSidebotham commented 3 years ago

@256dpi absolutely fine, the best fix should always win. Feel free to close the PR if it's of no use.