`volatile` bitfields must not be used as synchronization primites

xavi commented

2024-10-17 21:27:37 +00:00

struct HttpServer defines stop and isRunning as 1-bit volatile bitfields, presumably used as flags with atomic access.

However, the use of volatile variables on a multi-threaded environment is a wrong assumption. In C99, platform-specific primitives, such as pthread_mutex_t, must be used to protect variable access so that the operations are thread-safe.

If Cytoplasm is meant to keep using a multi-threaded HTTP server, these bitfields must be then replaced with thread-safe and portable alternatives.

`struct HttpServer` [defines](https://git.telodendria.io/Telodendria/Cytoplasm/src/commit/4f316ff7b3a955b831ca4aefb8679ddf3396a7d0/src/HttpServer.c#L57-L58) `stop` and `isRunning` as 1-bit `volatile` bitfields, presumably used as flags with atomic access. However, the use of `volatile` variables on a multi-threaded environment [is a wrong assumption](https://en.wikipedia.org/wiki/Volatile_variable#Multi-Threading). In C99, platform-specific primitives, such as `pthread_mutex_t`, must be used to protect variable access so that the operations are thread-safe. If `Cytoplasm` is meant to keep using a multi-threaded HTTP server, these bitfields must be then replaced with thread-safe and portable alternatives.

jordan commented

2024-10-17 23:47:44 +00:00

Owner

I actually am not so sure this is a big issue, because those variables are only ever modified by the main thread; it's not like they're being modified asynchronously. They're only written by one thread and then read by the others. So as far as multithreaded access goes, it's really not a problem. We aren't going to get any weird behavior due to context switches as far as I can tell.

In the case where the variable changes while it's being read by a thread, the worst that can happen is that one more loop happens, but that's not the end of the world because the new value will be picked up on the next iteration.

Locking a mutex every time we check for the shutdown condition seems like it would add a lot of overhead and somewhat kill the concurrency benefits of multithreading.

I actually am not so sure this is a big issue, because those variables are only ever modified by the main thread; it's not like they're being modified asynchronously. They're only written by one thread and then read by the others. So as far as multithreaded access goes, it's really not a problem. We aren't going to get any weird behavior due to context switches as far as I can tell. In the case where the variable changes while it's being read by a thread, the worst that can happen is that one more loop happens, but that's not the end of the world because the new value will be picked up on the next iteration. Locking a mutex every time we check for the shutdown condition seems like it would add a lot of overhead and somewhat kill the concurrency benefits of multithreading.

lda commented

2024-10-26 15:40:26 +00:00

Contributor

I actually am not so sure this is a big issue, because those variables are only ever modified by the main thread; it's not like they're being modified asynchronously.

I think that if so, then the HttpServer functions should have a note explaining that the start/stop primitives should be handled by the main thread.

> I actually am not so sure this is a big issue, because those variables are only ever modified by the main thread; it's not like they're being modified asynchronously. I think that if so, then the HttpServer functions should have a note explaining that the start/stop primitives should be handled by the main thread.

volatile bitfields must not be used as synchronization primites #58

`volatile` bitfields must not be used as synchronization primites #58