keepalive failed by calling MQTTYield with short timeout and short interval.

[aiot-embedded]

Hi,

In case of expiring last_receive timer, calling MQTTYield with short timeout and short interval will session close.

test code is as below.

void testcode(const char *topic, const char *payload)
{
  MQTTClient c=DefaultClient;
  MyMQTTClientInitialize(&c);

  MQTTPacket_connectData con = MQTTPacket_connectData_initializer;
  conData.keepAliveInterval = 60;
  MQTTConnect(&c,&con);

  message.qos       = QOS0;
  message.payload   = payload;
  message.payloadlen= strlen(payload);
  MQTTPublish(&c, topic, &message);

  sleep(60); //(1)

  MQTTYield(&c, 10); // (2)
  MQTTYield(&c, 10); // (3)
}

(1)

• Expire last_sent and last_receive timer in this sleep() for TEST.

(2) First MQTTYield call

• Send PINGREQ due to both of last_sent and last_receive timer has expired.
• But only last_sent timer is reseted.

(3) Second MQTTYield call

• PINGRESP is not coming yet and readPacket() return with timeout. (but it will come soon.)
• In keepalive(), last_receive timer has remain expired and ping_outstanding is1 so keepalive() return FAILURE and session will closed.

Client should wait more time to receive PINGRESP.
So, after sending PINGREQ client should also reset last_receive timer.

Thank you.

[alireza-tabatabaee]

@aiot-embedded Thanks for your suggested fix! However I think resetting the timer is too extreme, starting a new countdown after sending the ping packet seems to work flawlessly for me.

int keepalive(MQTTClient* c)
{
    int rc = SUCCESS;
    if (c->keepAliveInterval == 0)
        goto exit;
    if (TimerIsExpired(&c->last_sent) || TimerIsExpired(&c->last_received))
    {
        if (c->ping_outstanding)
            rc = FAILURE; /* PINGRESP not received in keepalive interval */
        else
        {
            Timer timer;
            TimerInit(&timer);
            TimerCountdownMS(&timer, 1000);
            int len = MQTTSerialize_pingreq(c->buf, c->buf_size);
            if (len > 0 && (rc = sendPacket(c, len, &timer)) == SUCCESS) // send the ping packet
            {
                TimerCountdownMS(&c->last_received, 3000);      // ADDED NEW
                c->ping_outstanding = 1;
            }
        }
    }
exit:
    return rc;
}

Still a weird decision by the pahoMQTT team. I don't know why they thought that PINGRESP would be available on the next run of the cycle() function, it might take up to a few seconds for the PINGRESP to arrive to the client when the network is slow (up to 2 seconds in my case)

[aiot-embedded]

@alireza-tabatabaee
Your code is so nice! It works fine for me too.

[icraggs]

The C version of the code was done by another contributor. It should match the C++ version, and it's definitely strange to expect a response within 2 seconds. I assume that was done for a particular application, and then no further thought was made. Anyway, I'll put a fix in which makes the C processing match the C++ code.

[alireza-tabatabaee]

it's definitely strange to expect a response within 2 seconds. I assume that was done for a particular application, and then no further thought was made.

Actually, the 3 second delay in the code I posted above was added by me as a makeshift patch. The way the original pahoMQTT C library works is that it'll send the PINGREQ packet, then on the next iteration of the cycle() / MQTTYield() function it expects to have the PINGRESP response from the broker. If it doesn't, it'll declare the connection as broken.

So, in use cases such as mine where the network was slow and PINGRESP packets might've taken a while (up to 2 seconds) to arrive at the device, the pahoMQTT library would just report the connection as broken on every ping event. Adding a little bit of leniency on the packet arrival time and not expecting a response right afterwards would fix it.

Nevertheless, thanks for your time and looking forward to your patch 🙏