Issue with latest lwmqtt and the separation of the header and the payload

[gonzabrusco]

Hello @256dpi . I think I've found a kind of a problem with the latest implementation of lwmqtt on a busy broker.

When you added the possibility of sending payloads directly from a user buffer, you splitted the lwmqtt_publish in two lwmqtt_write_to_network (one for the header and one for the payload) and thus the broker receives two TCP packets instead of one. This is showing to be a problem on a busy broker where a packet loss is expected because if the header is lost for some reason, then the broker will receive only the payload and interpret it as a malformed packed and disconnect the client.

Take a look at the following capture from Wireshark. There's always two TCP packets for every MQTT publish.

At first it seemed a very good idea to split the payload from the header. But this is now giving me problems. I'm not sure if the solution is to revert that functionality in lwmqtt or make it configurable.... or even better, send them together somehow in just one transfer (I think this is not possible, at least in Arduino).

[gonzabrusco]

After a long thought, I'm almost sure that this is not bug. I've been further reading and it seems MQTT should deal correctly splitted MQTT messages (in several TCP packets).

Maybe one feature that would be really nice is to be able to specify the maximum payload you want to send out. Sometimes you are using a connection that has a maximum payload (for example NB-IoT) and you are forced to split the payload in serveral TCP packets. It would be nice to be able to tell the publish method the maximum payload of each client-write(). And if you set it to zero, it means "no limit" (for standard wifi connections).

[gonzabrusco]

More findings: In Arduino (at least on ESP32) there's no way of knowing the size of the packages the stack tcp ip will send. No matter if you split the client->write() into multiple calls, it will send all together because it seems the ESP32 fills an output buffer instead of sending it instantly.

Nevertheless, the implementation change of splitting the payload in chunks turned out useful (at least with a SIMCOM) because some modems have a limited input buffer and you can't send unlimited data at once. Thus, in that case I changed you library to be able to configure the chunk size in the publish() function. Something like this (if you are interested, I can make a merge request in lwmqtt).

  // send payload if available
  if (msg.payload_len > 0) {
    if(max_payload_chunk_size == 0) {
      err = lwmqtt_write_to_network(client, msg.payload, msg.payload_len);
      if (err != LWMQTT_SUCCESS) {
        return err;
      }
    } else {
      uint32_t sent = 0;
      uint32_t chunk_size;
      while(sent < msg.payload_len) {
        if(msg.payload_len - sent >= max_payload_chunk_size) {
            chunk_size = max_payload_chunk_size;
        } else {
            chunk_size = msg.payload_len - sent;
        }
        err = lwmqtt_write_to_network(client, msg.payload + sent, chunk_size);
        if (err != LWMQTT_SUCCESS) {
          return err;
        }
        sent += chunk_size;
      }
    }
  }

[256dpi]

@gonzabrusco Thanks for the detailed report of your investigation!

However, I think that this should not be implemented in the scope of this library or the lwmqtt project. For both projects there is an elegant way to implement such custom networking behaviour. For arduino-mqtt you could simply implement your own Client class that performs the custom buffering. In lwmqtt, one would need to set a custom network write callback that implements the logic.

Also: If a modem silently drops data because its buffers are full and the network stack does not perform the proper TCP packet re-sending logic, you have a lot more problems to worry about.

[gonzabrusco]

You are completely right. I will keep this implemented in my own fork.