Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Is it safe to call coreMQTT APIs from a user callback? #278

Closed
hlef opened this issue Apr 10, 2024 · 4 comments
Closed

Is it safe to call coreMQTT APIs from a user callback? #278

hlef opened this issue Apr 10, 2024 · 4 comments

Comments

@hlef
Copy link

hlef commented Apr 10, 2024

Hi,

Is it safe for a user callback to call a coreMQTT API?

I seem to be seeing that calling MQTT_ProcessLoop from a user callback wouldn't be safe if the nested call hits an error path and calls discardStoredPacket (here), setting the index to 0 (here), and causing an overflow in the parent call (here), but I hope to be wrong.

@AniruddhaKanhere
Copy link
Member

AniruddhaKanhere commented Apr 10, 2024

Hello @hlef,

The user callback is called from within MQTT_ProcessLoop. So, if you call coreMQTT APIs from within the callback, it might cause a nested calling of MQTT_ProcessLoop as MQTTProcessLoop -> callback -> MQTT_ProcessLoop ... which might lead to increased stack consumption in the least.

And in the worst case, as you pointed out, there can be data corruption as one call to ProcessLoop does one thing, whereas the nested call changes a value in use by the parent call.

The coreMQTT library is not designed to be thread safe to keep it simple and light weight. It is recommended to allow only one thread to call MQTT APIs or to protect them with mutex if more than one threads are going to be calling the APIs simultaneously.

What is your use case? Why do you want the API to be called inside the callback?

Thanks,
Aniruddha

@hlef
Copy link
Author

hlef commented Apr 11, 2024

Hi Aniruddha,

Thanks for the quick answer. We have a scenario where we want to do a publish in response to an incoming publish event.

It is quite convenient to do the publish in the incoming publish callback. If that is not safe we will set some flags and perform publishes once we are out of the process loop.

Thanks!

@AniruddhaKanhere
Copy link
Member

Hello Hugo,

Thank you for that information!

Yes, it will not be safe to call MQTT API from the callback itself. As you mentioned, setting some flags would be helpful. Ideally, I would not put too many things in the callback. I'd much rather have the handler copy data into a buffer or put it in a queue (both options have their pros and cons) which can be used to 'kick' a different DataProcessing task into action to process the data.

I say that because doing multiple things in the callback might cause MQTT ProcessLoop not being called enough. Additionally, the processing of the data will happen at the priority of the task calling ProcessLoop - you may or may not want that. Having a different task will let you process the data when other high priority tasks are sleeping or blocked.

But, of course I do not know your whole design - so take my comments as 'best practices' instead of something you must do.

That being said, seems that I have answered your question and will be closing this issue. If that is not the case, feel free to re-open the thread (or create a new one). :)

Thanks,
Aniruddha

@hlef
Copy link
Author

hlef commented Apr 18, 2024

That answers my question, thanks! I think that it would be helpful to mention some of this in the documentation of the user callback type (for example as a note).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants