[wasmfs] Fix data race in thread initialization. #25114
Conversation
The `ProxyWorker` was creating a thread in the constructor initializer list and using a captured reference to the `started` member variable. This is problematic because it is not guaranteed that the class has been fully constructed during the initializer list. What happens: 1) `ProxyWorker` initializers run 2) Thread starts and sets `started = true` 3) `ProxyWorker` finishes construction and sets `started = false` 4) `ProxyWorker` waits for `started` to be `true` Step 4 will never finish in this case since the thread already ran. To fix this we simply need to move the thread creation into the constructor body where the class is guaranteed to be fully constructed. Fixes emscripten-core#24370, emscripten-core#24676, emscripten-core#20650
| started = true; | ||
| } | ||
| cond.notify_all(); | ||
| : queue() { |
There was a problem hiding this comment.
Does clang-format not put this onto a single line now?
|
Nice work!!!! |
|
As mentioned in chat, I tried fixing this by moving Anyone have a preference on solution? I lean towards the current PR since any new members and dependencies will work as expected. |
|
I think using the correct decl order is better, but please also a comment about the decl order being important. |
|
Actually maybe apply both fixes, since didn't you also read the starting threads in initializers is bad? Either way please add comments in the decl order. |
| std::unique_lock<std::mutex> lock(mutex); | ||
| started = true; |
There was a problem hiding this comment.
I suspect this is still technically incorrect because the initialization write of started is not protected by the mutex, so it will not synchronize with this mutex-protected read on the worker thread. (Unless the std::thread constructor introduces some synchronization, but the documentation doesn't say.) Let's get rid of the lock and make the bool atomic in addition to the changes here.
There was a problem hiding this comment.
From what I interpret from the class initialization rules, started is guaranteed to be initialized before the thread is created, so we shouldn't need a lock at that point.
The
ProxyWorkerwas creating a thread in the constructor initializer list and using a captured reference to thestartedmember variable. This is problematic because it is not guaranteed that the class has been fully constructed during the initializer list.What happens:
ProxyWorkerinitializers runstarted = trueProxyWorkerfinishes construction and setsstarted = falseProxyWorkerwaits forstartedto betrueStep 4 will never finish in this case since the thread already ran.
To fix this we simply need to move the thread creation into the constructor body where the class is guaranteed to be fully constructed.
Fixes #24370, #24676, #20650