Incoming messages can be lost

Hi. First of all, thanks for this library, it's mostly been very reliable and I love the configurability and control.

However I've found a problem that can result in incoming MQTT messages being permanently lost.

I am using the latest release v1.5.1 in my project.

The issue arises when all of the following apply:

Client is reconnecting to a broker it has previously used, using cleansession=false and reusing an existing client id.
Client subscribes to more than one topic, or to a different topic than it did in the previous session.
One or more QoS 1 or 2 messages to one or more of the client's subscribed topics from its previous session, other than the first one it subscribes to in this session, are enqueued on the broker.

When a client is reconnecting a session with cleansession=false, the broker does not wait until the client resubscribes to a topic to send it messages for that topic; as soon as it connects, it immediately starts to send any queued messages for the client's subscriptions from its previous session.

ArduinoMqtt handles this ok if the client only ever subscribes to one topic, because there is no opportunity between connect() and subscribe() for the incoming messages to be processed. So it calls subscribe() for the topic, sets up the message handler, and then processes the incoming messages, no problem.

But if there is more than one subscribed topic, or the topic has changed, when subscribe() is called for the first time it will start to receive and process any queued messages for other topics as well. It acknowledges receipt of each message, and then calls deliverMessage() - which finds that there is not (yet) a handler defined for that topic, so it just says "Unexpected message" and throws it away!

There is a workaround, I think: instead of waiting until subscribe() to set up the message handlers, one can set them up beforehand, by explicitly calling mqttMessageHandlers->set(topic, callback) for each topic. However the necessity of doing this is not obvious, and it makes the requirement to specify the same callback to subscribe() as well seem a bit extraneous.

To fix this properly would, I think, need a breaking change - but that might be justified to raise awareness and prevent others falling victim to this issue. It could be done by, for example, requiring subscribe() is called before connecting. It would set up the message handlers and keep a list of topics to be subscribed to, and the actual subscription requests would be sent on connect. Or, remove the cbk argument from subscribe(), and require that all message handler callbacks are registered before connect or before the first call to subscribe() - but that seems harder to enforce.

Regardless, I think processPacket() must not send a PUBACK or PUBREC for an incoming PUBLISH packet if there is no message handler defined for the topic. This is a general matter of safety, not specific to this issue - acknowledging receipt of a message only to then throw it away without delivering it, is not the behaviour most people would expect. If you can't deliver it (because there's no handler, or because of any other temporary error) then don't ack it, so the broker will try again. If the user wants to discard certain messages, they can choose to do that, but the library should never do so by default! Making this fix should not break anything, and would at least ensure that the broker should attempt redelivery later, giving us time to complete all the subscriptions - however, it's possible that if we only make short-lived MQTT connections, the broker will simply keep trying to deliver the message at the wrong time, and so although it isn't lost as such, we will never receive it properly.

I hope that all makes sense. If you can think of a better solution I'm all ears. I was going to simply make a PR but I thought this probably warrants some discussion first about the right approach.

Cheers Ben

monstrenyatko / ArduinoMqtt

Incoming messages can be lost #33