amcolex
commented
10 months ago Hi @csobrinho, thanks for taking a close look!
Regarding the concurrency protection for the message instances: In our applications (embedded/STM32), we adhere to a basic principle of not having any runtime mallocs, which means all of our subscribes are done before the scheduler starts. However, you're correct that this could cause issues if done at runtime. Adding a mutex lock here would be a free and easy fix.
As for the peek, I actually hesitated about adding the mutex protection. The reason I refrained was to avoid the extra CPU overhead, since mutexes can be expensive when used liberally. Also, in practice, having the data partially update while reading is often a non-issue. It's a pretty common design pattern in our codebases. But I do agree that it would be cleaner and more correct to add the mutex lock to the peek and publish functions.
It's a small change, and I'll look into it.
Cheers!
Hi @Amcolex, I was looking through the code and saw a bunch of interesting ideas, especially the gen side for the boilerplate messages. Overall it's well done but a few things caught my attention and wanted to know what were your thoughts about concurrency.
I see each .c contains n instance messages. Each message metadata contains a single linked list for the subscription. However the sublist is not protected by a lock so technically it is possible you can end up with a memory leak + missed subscription because you malloc and potentially add to a list that can be changed by two threads at the same time.
Another issue is with the peek that reads the counter (or overall any method that reads the instance values and does something with it). If the counter > 0 and while you copy the value is changed then you might memcpy some random data.
I think the library needs some sort of synchronization + a lock object on the msg instance and I'm happy to try to send you a PR later this year if you are interested.
Thanks!
Hi @Amcolex, I was looking through the code and saw a bunch of interesting ideas, especially the gen side for the boilerplate messages. Overall it's well done but a few things caught my attention and wanted to know what were your thoughts about concurrency.
I see each .c contains n instance messages. Each message metadata contains a single linked list for the subscription. However the sublist is not protected by a lock so technically it is possible you can end up with a memory leak + missed subscription because you malloc and potentially add to a list that can be changed by two threads at the same time.
Another issue is with the peek that reads the counter (or overall any method that reads the instance values and does something with it). If the counter > 0 and while you copy the value is changed then you might memcpy some random data.
I think the library needs some sort of synchronization + a lock object on the msg instance and I'm happy to try to send you a PR later this year if you are interested.
Thanks!