bertmelis / espMqttClient

MQTT 3.1.1 client library for the Espressif devices ESP8266 and ESP32 on the Arduino framework.
https://www.emelis.net/espMqttClient/
MIT License
100 stars 21 forks source link

[BUG] Out of order PUBACKs are ignored #24

Closed serfreeman1337 closed 2 years ago

serfreeman1337 commented 2 years ago

Describe the bug Out of order PUBACKs are being ignored.

For QOS 2 publishes that were sent in loop I got packet ids in order 3, 4, 5, 6, 7, but from MQTT server I got out of order PUBACKs: 3, 6, 5, 7, 4

Resulting onPublish was only called for packet id 3 and 4.

image

Observed on ESP8266 using Non TLS version. Arduino IDE with latest esp8266 board library.

Expected behaviour onPublish should be called for every PUBACK regardles of order.

serfreeman1337 commented 2 years ago

Simple commenting out this break made it work for me: https://github.com/bertmelis/espMqttClient/blob/835bcfb16853cd19637c2f7001df24320425cfb2/src/MqttClient.cpp#L486

I see that QOS 2 behavior is differs from async-mqtt-client. This library sends QOS 2 publish right away, while async-mqtt-client waits for PUBACK before sending anything else.

espMqttClient ![image](https://user-images.githubusercontent.com/2133936/189480350-5c9ac591-344a-4f70-bd8d-ab08c8b4f307.png)
async-mqtt-client ![image](https://user-images.githubusercontent.com/2133936/189480403-fbdbcc3f-c6ef-41f8-a53c-ea3ca1f0098b.png)
bertmelis commented 2 years ago

PUBACKS should be sent in the order in which the PUBs are sent. That is a MUST: http://docs.oasis-open.org/mqtt/mqtt/v3.1.1/os/mqtt-v3.1.1-os.html#_Toc398718105 That is why the callbacks are not called.

I didn't implement retry yet because I've never seen this behaviour. I'll put it on the top of the TODO list.

And indeed, this lib doesn't wait for PUB packets to be acknowledged before sending new ones. It's to speed up things.

Do you also have a wireshark when the PUB packets were sent by the client (or received by the broker)?

bertmelis commented 2 years ago

Hmmm, I might be mistaken here. The spec talks about MUSTs for clients. The broker has more freedom.

serfreeman1337 commented 2 years ago

I'm using ThingsBoard as mqtt server. I've checked traffic on the server side and indeed it's mqtt server which sends out of order PUBACKs.

wireshark on mqtt server side ![image](https://user-images.githubusercontent.com/2133936/189481697-95f81f86-e3cc-47b7-ae3a-e106d2890bc2.png)
bertmelis commented 2 years ago

I created a branch/PR to remove the message ordering check. I reread the specification and concluded the ordering is not mandatory for the broker. So, the client has to accept the out of order ACKs. I assume you agree? image

bertmelis commented 2 years ago

Now, it's more complicated than this. If in the middle of this unordered acking, the connection interrupts, the client still has to resend the original messages in the same order. the PR is therefore not final because this requirement is not met anymore.

serfreeman1337 commented 2 years ago

I found that this issue only happens with ThingsBoard's "Gateway API" topics (v1/gateway/*). With default topics (v1/devices/me/*) all PUBACKs are in order, even if I publish more than 150 messages at once.

So, it's more thingsboard server issue. I'll implement queing for gateway topics myself then.

Thank you for pointing out that it might be a server problem.

bertmelis commented 2 years ago

In this case the client in too strict. I'll solve it in the next release. IT wiill take some time to implement "retransmission".

bertmelis commented 2 years ago

Closing this in favour of #46.