MQTT Connection Status Thread Safety

[DakshitBabbar]

Description

Following is a brief summary of changes:

1. Check the connected flag before doing any send operation on the connection
2. Make all the APIs that do send operations, thread safe
3. Update the connected flag within MQTT_Disconnect regardless of the return status of the send operation

Following are the specifics of the changes:

1. Add 3 new MQTTStatus_t values: MQTTStatusConnected, MQTTStatusNotConnected and MQTTStatusDisconnectPending
2. Added 1 new MQTTConnectionStatus_t value: MQTTDisconnectPending
3. Update the MQTT_Status_strerror function to handle the new MQTTStatus_t values
4. Add a new API function MQTT_CheckConnectStatus() that will check the value of the context→connectStatus flag safely.
5. Add this API to the core_mqtt.h file to make it available to users
6. Check the connected flag before doing any Send operation (following API's are updated)
   a. sendPublishAcks
   b. MQTT_Connect
   c. MQTT_Subscribe
   d. MQTT_Publish
   e. MQTT_Ping
   f. MQTT_Unsubscribe
   g. MQTT_Disconnect
7. Use the MQTT_PRE_STATE_UPDATE_HOOK() and MQTT_POST_STATE_UPDATE_HOOK() to make the send APIs thread safe
8. The connect status is set to MQTTDisconnectPending whenever a transport send or receive function returns a negative error code
9. const keyword for the the MQTTStatus_t is removed in the input parameters for the receive functions as we need to update the connection status when the receive function returns a negative error code

Relevant Explanations

• MQTT_PRE_SEND_HOOK(): The Pre and Post Send hook Macros are not required now, as the sending logic will be within the pre and post state update hook itself. (because we cannot allow other threads to change the connection state of the application until a send operation is complete).
• I have split the handleSessionResumption function. The part of that function which was handling the clean session has been added within the mutex calls in the MQTT_Connect API and the unclean session part is handled by this new function that is called outside the mutex calls.

Pending Tasks

• Doxygen example for the new API
• Unit Test Updates
• CBMC Proof

[amazonKamath]

I think we dont need example for this case or do you plan to addone?

[DakshitBabbar]

I am yet to write an example for this. If we do not need it then I can skip it. Please let me know.

[AniruddhaKanhere]

Thank you for this Pull Request @DakshitBabbar!

I have left a few comments. Can you please take a look?

Also an overarching comment. We seem to be holding the mutex for a lot longer. If not absolutely necessary, please relinquish the mutex and take it back again when executing a critical section. This allows other threads to take the mutex in between the giving and taking of the mutex by the current task (think of a high priority task waiting on the mutex). In such cases, priority inversion happens, but lets try and keep it as short a duration as possible.

Thanks again!

[AniruddhaKanhere]

Why can we not do this check even before calling MQTT_GetConnectPacketSize?

[DakshitBabbar]

Reasoning I applied:
If the check failed then it would be good that we will exit early, but if it passed then the Mutex will be held for a longer period of time, unnecessarily.

But yes one thing can be done:
Take the mutex, do the check in the beginning, if fails, change status and bubble it up, else relinquish the mutex and take it back when needed again. Is this what we want to do?

EDIT: We should not relinquish the mutex after checking the state as the state might be changed by some other thread and when this thread continues it might assume that the state is still connected and try to send the packet. Hence if we put this check before MQTT_GetConnectPacketSize then the mutexes will contain unnecessary code, which we do not want. Therefore I think this is a good approach.

please let me know otherwise. Thanks!

[AniruddhaKanhere]

I realize that this was already there, but why not fix it when we are modifying the library for the better :)

[DakshitBabbar]

fixed

[DakshitBabbar]

/bot run formatting

[AniruddhaKanhere]

Few minor comments. Thank you for your patience @DakshitBabbar

[AniruddhaKanhere]

Curious why this change was required. Not a blocker, I can approve with or without this change but just curious.

[DakshitBabbar]

This was required because while building unit tests we were having some errors on mac. So @aggarg suggested this change along with many more to resolve them.

[DakshitBabbar]

/bot run formatting