multiple retained messages for the same topic

[real-bombinho]

TinyMqtt.cpp, line 965:

auto old = retained.find(topic);

does not work as expected. Consequentially I get multiple retained messages for the same topic.

Arduino 2.1.0

Also Retain seems to store the whole Publish message instead of just the message string, which is quite a bit of overkill (memory consuming) at long topics. In accordance to HiveMQ (https://www.hivemq.com/blog/mqtt-essentials-part-8-retained-messages/) it should just be the QOS and the message string itself.
Would it not be better to just store a reference to the topic?
Then duplicates would be far less likely to occur by design and the load on the memory would be lesser. Also the handling of empty messages (delete/ignore) would be much easier.

To replicate the issue, initialise MqttBroker with a value >1 for retained values and send multiple retained messages to the same topic. Connect a client and subscribe to the topic. Now you should see all retained messages popping up, instead of just one.

I have traced the error to the aforementioned line and a debug output there reveals that [topic] is never found. Using a quoted string as argument on the other side does return a found result.

[hsaturn]

Hello

I've modified the unit test on retained messages... Unable to reproduce this.

The test added is there 30e75b8

Either you do not use the main branch, or .... the map is buggy on ESP implementation.

[hsaturn]

Also this gave me the opportunity to add a test for the payload truncated message !!!

[real-bombinho]

The std::map seems not to recognise any values unless I use hardcoded quoted strings.

[hsaturn]

The commit for the test added is there (30e75b8)

[hsaturn]

The std::map seems not to recognise any values unless I use hardcoded quoted strings.

Yes, this sounds very strange.

[real-bombinho]

Are you using the new 2.0.x environment?

I had compiled it for RP2040 and ESP8266, both behave the same.

[hsaturn]

No. Unit test are not running on the ESP, but on Linux. This is why I suspect the map to be responsible of this :-(

[cdconner16]

I can confirm the multiple messages for one topic with my ESP32 on the latest version of this library.
For the broker, I'm using simple-broker.ino with a couple modifications to the console and RETAIN = 10. Note that I do not fully understand MQTT yet so please correct me if I'm wrong about how it should work.

Last 16 publishes from my client with retain always set to true:
topic: thermostat/set_temp, payload: 71.00
topic: thermostat/set_temp, payload: 72.00
topic: thermostat/mode, payload: heat
topic: thermostat/relay_state_comp, payload: On
topic: thermostat/relay_state_rv, payload: On
topic: thermostat/state, payload: Heat
topic: thermostat/mode, payload: cool
topic: thermostat/mode, payload: off
topic: thermostat/relay_state_comp, payload: Off
topic: thermostat/relay_state_rv, payload: Off
topic: thermostat/state, payload: Off
topic: thermostat/mode, payload: heat
topic: thermostat/state, payload: Waiting_Comp_Off_Time
topic: thermostat/mode, payload: cool
topic: thermostat/mode, payload: off
topic: thermostat/state, payload: Off

After resetting my client and resubscribing to the topics I get the following output:
topic: thermostat/mode, payload: heat
topic: thermostat/mode, payload: cool
topic: thermostat/mode, payload: off
topic: thermostat/state, payload: Off
topic: thermostat/state, payload: Heat
topic: thermostat/state, payload: Off
topic: thermostat/relay_state_comp, payload: On
topic: thermostat/relay_state_comp, payload: Off
topic: thermostat/relay_state_rv, payload: On
topic: thermostat/relay_state_rv, payload: Off

However, I've lost the last value for the thermostat/set_temp topic because of the behavior. I would expect only the last value for each topic to be retained. I would also expect a topic to be completely removed from the buffer only if there are more than 10 topics that are trying to retain values.

[cdconner16]

Trying to debug this because it's the last piece of code I need to make my system work.
The first problem is happening at line 977 in TinyMqtt.cpp, where retained.find(topic) isn't working for topics that already exist so we always take the if case and go into retainDrop().

Digging further, Topic is a subclass of IndexedString and its == operator is looking at the index value. By making index public I probed it while sending multiple publish with retain requests and it keeps incrementing. So the same string is getting different indexes...

It also seems that the client subscription list uses Topic.matches() which will try to use indexes for quick matches but will fallback to the strings. So that's why the clients are still receiving topics they are subscribed too even though the indexes keep changing.

So a big problem is happening in StringIndexer.h strToIndex(). The real problem maybe happening at a higher level but I am not ever entering the following if statement. So for the same topic, I'm never getting the same indexed string, so the map for retained can't == to previous topics.
if (it->second.str.length() == len && strcmp(it->second.str.c_str(), str)==0)

So I think the fundamental problem is line 77 in StringIndexer.h.
The str char array passed to this function actually contains the full message, not just the topic so strcmp is not returning 0 because of that. Changed line 77 to the following and fixed the string matching:
if (it->second.str.length() == len && strncmp(it->second.str.c_str(), str, len)==0)

Now to go back and check functionality.

Looks like that did it!