Timeout weirdness in mqtt_run loop

[rmn30]

I noticed some weirdness in the mqtt_run function. Specifically:

1. in the condition of the while loop

   network-stack/lib/mqtt/mqtt.cc

   Line 1115 in a7314f9

   } while (t->remaining > 0 && status != MQTTSuccess);

   I don't think status != MQTT_Success is ever true because if it were we would've hit one of the return statements above. This means the loop always executes exactly once, so what's the point of having a loop?! For the same reason I don't think the return at

   network-stack/lib/mqtt/mqtt.cc

   Line 1119 in a7314f9

   return -ETIMEDOUT;

   is ever reachable so the function will never return -ETIMEDOUT.

2. Relatedly, although the function takes a Timeout and elapses it, it will call MQTT_ProcessLoop (which I presume may block?) even if timeout->may_block() is false. Is this the correct behaviour?

I noticed this because this function is called in a loop in the Hugh demo here and I noticed that it never timed out in long runs even though the timeout was never reset. This is harmless in this case and actually what we want, but it's weird.

[venngar]

I don't think status != MQTT_Success is ever true because if it were we would've hit one of the return statements above

Not necessarily - there seems to be one case when you shouldn't exit from the loop (TLS link is active and attempt to reconnect):

network-stack/lib/mqtt/mqtt.cc

Lines 1085 to 1087 in a7314f9

	// If the TLS link is still live, try
	// again until we are out of time.
	if (connection->networkContext.isDisconnected)

However I think that status != MQTTSuccess check within while condition is unnecessary at all - because if the if (status != MQTTSuccess) is true and one of the return statements is hit then the only thing we need to monitor is t->remaining > 0.

[venngar]

Had another look at the code. I think removing status != MQTTSuccess from a while loop condition (which is not great but harmless) and replacing t->remaining > 0 with t->may_block() for better code formatting, I think it seems works as expected.

This line:

network-stack/lib/mqtt/mqtt.cc

Line 1119 in a7314f9

return -ETIMEDOUT;

can be reached if processing received packets doesn't result in any errors (either due to packet structure or issues with sending and receiving at the network-stack end) but the timeout was elapsed (therefore exiting while loop without if statement):

network-stack/lib/mqtt/mqtt.cc

Lines 1069 to 1074 in a7314f9

	status = with_elapse_timeout(t, [&]() {
	// `MQTT_ProcessLoop` handles keepalive.
	return MQTT_ProcessLoop(coreMQTTContext);
	});
	if (status != MQTTSuccess)

As for the second point: in its current implementation it looks as intended behavior because timeout->may_block() condition is checked after the MQTT_ProcessLoop is called.