espressif / esp-mqtt

ESP32 mqtt component
Apache License 2.0
591 stars 254 forks source link

Retransmission of QOS0 (IDFGH-12829) #276

Closed Totrasmek closed 1 month ago

Totrasmek commented 1 month ago

Description

Currently in the state MQTT_STATE_CONNECTED the client will, on successful mqtt_resend_queued, set the pending status of the current pending msg id to TRANSMITTED. The goal of this to trigger the retransmission logic. However, retransmission is not necessary for QOS0 packets, and as QOS0 packets all have the same msg_id (0) and are deleted upon a successful mqtt_resend_queued, this can lead to a different item in the outbox being set to TRANSMITTED. This can cause items in the outbox to send in a weird order. Let me know if my understanding is incorrect.

Code

From mqtt_client.c line 1671

            // resend all non-transmitted messages first
            outbox_item_handle_t item = outbox_dequeue(client->outbox, QUEUED, NULL);
            if (item) {
                if (mqtt_resend_queued(client, item) == ESP_OK) {
                    outbox_set_pending(client->outbox, client->mqtt_state.pending_msg_id, TRANSMITTED);
                }
                // resend other "transmitted" messages after 1s
            } else if (has_timed_out(last_retransmit, client->config->message_retransmit_timeout)) {
                last_retransmit = platform_tick_get_ms();
                item = outbox_dequeue(client->outbox, TRANSMITTED, &msg_tick);
                if (item && (last_retransmit - msg_tick > client->config->message_retransmit_timeout))  {
                    mqtt_resend_queued(client, item);
                }
            }

Suggested Change

I believe that the line outbox_set_pending(client->outbox, client->mqtt_state.pending_msg_id, TRANSMITTED); should only be called if the item's QOS != 0.

euripedesrocha commented 1 month ago

Hi @Totrasmek thanks for reporting this. Should be fixed soon.