knolleary / pubsubclient

A client library for the Arduino Ethernet Shield that provides support for MQTT.
http://pubsubclient.knolleary.net/
MIT License
3.78k stars 1.46k forks source link

[MQTTPINGREQ] lastOutActivity is not set when replying to ping req. #1059

Open TD-er opened 1 week ago

TD-er commented 1 week ago

Use case is a client connected to the broker without sending a lot of messages.

Typically the client will send out a keep-alive ping (MQTTPINGREQ) to the broker and the broker will reply to it with a MQTTPINGRESP However if the broker sends out such a MQTTPINGREQ first, the client replies with a MQTTPINGRESP but doesn't set the lastOutActivity.

I assume the broker may not reply to a MQTTPINGREQ if it is sent very shortly after the last keep-alive (requested by the broker) was dealt with and acknowledged.

These kind of disconnects seem to happen mainly recently as I got quite a few reports lately where it seemed to be "fixed" by sending out more messages to keep within this keep-alive interval. So I assume there are some MQTT brokers out there which may have implemented such a rate-limiter feature (or only now enabled by default?) on keep-alive pings.

See: https://github.com/knolleary/pubsubclient/blob/2d228f2f862a95846c65a8518c79f48dfc8f188c/src/PubSubClient.cpp#L419-L423

Suggested fix:

 } else if (type == MQTTPINGREQ) { 
     this->buffer[0] = MQTTPINGRESP; 
     this->buffer[1] = 0; 
     if (_client->write(buffer,2) != 0) {
         lastOutActivity = t;
     }
 } else if (type == MQTTPINGRESP) { 

N.B. the same issue here, when MQTT_MAX_TRANSFER_SIZE is defined https://github.com/knolleary/pubsubclient/blob/2d228f2f862a95846c65a8518c79f48dfc8f188c/src/PubSubClient.cpp#L581-L603

Suggested fix:

    while((bytesRemaining > 0) && result) {
        delay(0);  // Prevent watchdog crashes
        bytesToWrite = (bytesRemaining > MQTT_MAX_TRANSFER_SIZE)?MQTT_MAX_TRANSFER_SIZE:bytesRemaining;
        rc = _client->write(writeBuf,bytesToWrite);
        result = (rc == bytesToWrite);
        bytesRemaining -= rc;
        writeBuf += rc;
        if (rc != 0) {
            lastOutActivity = millis();
        }
    }
    return result;