-
Notifications
You must be signed in to change notification settings - Fork 3.4k
[wasmfs] Fix data race in thread initialization. #25114
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -34,32 +34,36 @@ class ProxyWorker { | |
public: | ||
// Spawn the worker thread. | ||
ProxyWorker() | ||
: queue(), thread([&]() { | ||
// Notify the caller that we have started. | ||
{ | ||
std::unique_lock<std::mutex> lock(mutex); | ||
started = true; | ||
} | ||
cond.notify_all(); | ||
: queue() { | ||
// Initialize the thread in the constructor to ensure the object has been | ||
// fully constructed before thread starts using the object to avoid a data | ||
// race. See #24370. | ||
thread = std::thread([&]() { | ||
// Notify the caller that we have started. | ||
{ | ||
std::unique_lock<std::mutex> lock(mutex); | ||
started = true; | ||
Comment on lines
+44
to
+45
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I suspect this is still technically incorrect because the initialization write of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. From what I interpret from the class initialization rules, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, initialized on the current thread, but that initialization write does not necessarily become visible on any other thread without some form of synchronization. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It took me a little while to dig this up, but thread creation is a synchronization point. Initializing (unguarded) data before thread creation is pretty common pattern, so I was going to be surprised if that was actually UB. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice find, that makes sense. |
||
} | ||
cond.notify_all(); | ||
|
||
// Sometimes the main thread is spinning, waiting on a WasmFS lock held | ||
// by a thread trying to proxy work to this dedicated worker. In that | ||
// case, the proxying message won't be relayed by the main thread and | ||
// the system will deadlock. This heartbeat ensures that proxying work | ||
// eventually gets done so the thread holding the lock can make forward | ||
// progress even if the main thread is blocked. | ||
// | ||
// TODO: Remove this once we can postMessage directly between workers | ||
// without involving the main thread or once all browsers ship | ||
// Atomics.waitAsync. | ||
// | ||
// Note that this requires adding _emscripten_proxy_execute_queue to | ||
// EXPORTED_FUNCTIONS. | ||
_wasmfs_thread_utils_heartbeat(queue.queue); | ||
// Sometimes the main thread is spinning, waiting on a WasmFS lock held | ||
// by a thread trying to proxy work to this dedicated worker. In that | ||
// case, the proxying message won't be relayed by the main thread and | ||
// the system will deadlock. This heartbeat ensures that proxying work | ||
// eventually gets done so the thread holding the lock can make forward | ||
// progress even if the main thread is blocked. | ||
// | ||
// TODO: Remove this once we can postMessage directly between workers | ||
// without involving the main thread or once all browsers ship | ||
// Atomics.waitAsync. | ||
// | ||
// Note that this requires adding _emscripten_proxy_execute_queue to | ||
// EXPORTED_FUNCTIONS. | ||
_wasmfs_thread_utils_heartbeat(queue.queue); | ||
|
||
// Sit in the event loop performing work as it comes in. | ||
emscripten_exit_with_live_runtime(); | ||
}) { | ||
// Sit in the event loop performing work as it comes in. | ||
emscripten_exit_with_live_runtime(); | ||
}); | ||
|
||
// Make sure the thread has actually started before returning. This allows | ||
// subsequent code to assume the thread has already been spawned and not | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does clang-format not put this onto a single line now?