soypat / natiu-mqtt

A dead-simple, extensible MQTT implementation well suited for embedded systems.
MIT License
87 stars 4 forks source link

Best practice when using with go routines #4

Open brutella opened 1 year ago

brutella commented 1 year ago

My MQTT client is using this library to abstract the communication with a broker. At first this library looks pretty straight forward to use, but I do find it hard to use with go routines.

Why go routines? Some parts of the MQTT protocol requires you to read and write simultaneously. For example once you are connected to a broker, you have to send a Ping message after n seconds to keep the connection alive. But also when you're subscribed to a topic, you have to keep reading from the connection using ReadNextPacket(). This means you have to use a go routine to either send or receive messages.

The problem is that when you call Ping() on a different go routine, you will get a data race – the Ping() implementation calls ReadNextPacket(). Within this method the LastReceivedHeader field is written – you get a data race because this field is written on 2 different go routines.

Any ideas on how to avoid that?

soypat commented 1 year ago

Hey again! Fixing this is high on my priority list this week. I'm thinking of breaking the Client API to reach a much better usage pattern- how would you feel about this?

My reasons for this change are because I wrote this library before learning about the connection state pattern which I feel would make the library more robust and easier to use concurrently like you say.

Basically this would allow you to call ping asynchronously without worrying about the race condition which would be taken care by the connection state type. I've started work on the ~dev~ client-rewrite branch

brutella commented 1 year ago

Fixing this is high on my priority list this week. I'm thinking of breaking the Client API to reach a much better usage pattern- how would you feel about this?

This sounds great. 👍 Please let me know when the refactoring is done so that I can take a closer look.

soypat commented 1 year ago

Refactoring is done- I've yet to test changes though. Take a look over at #5

soypat commented 1 year ago

@brutella Just curious- did you manage to get up and running with the PR?