Repository navigation
Conversation
There was a problem hiding this comment.
Code Review
This pull request implements blocking support for poll() and select() in WasmFS by introducing wait queues (PollWaiter, WaitQueue, and WaitRegistration). Pipes are updated to utilize these queues to notify waiting threads of state changes, and __syscall_poll is refactored to block when a timeout is specified. Consequently, several blocking select and poll tests have been re-enabled. The feedback suggests optimizing __syscall_poll by calling reserve(nfds) on the files and regs vectors to prevent multiple dynamic allocations and moves of WaitRegistration objects.
Part of emscripten-core#26736, Asyncify/JSPI support will follow
9a13164 to
196182f
Compare
b64f10e to
daa6bd7
Compare
daa6bd7 to
cf84030
Compare
There was a problem hiding this comment.
Code Review
This pull request implements blocking poll() support in WasmFS using wait queues, allowing threads to block until a file descriptor becomes ready. It introduces PollWaiter, WaitQueue, and WaitRegistration classes, integrates them with PipeFile and __syscall_poll, and adds JS library helpers to support asynchronous polling on the main browser thread. The review feedback highlights a critical race condition (lost wakeup) between _wasmfs_poll_wait and _wasmfs_poll_notify when yielding via Asyncify on the main thread. To resolve this, a 'pre-triggered' state should be supported in wasmFSPollWaiters to capture notifications that arrive before the wait is registered, and both functions should declare $wasmFSPollWaiters in their __deps.
| _wasmfs_poll_wait__async: 'auto', | ||
| _wasmfs_poll_wait__sig: 'ipi', | ||
| _wasmfs_poll_wait: (key, ms, sync) => { | ||
| if (sync) { | ||
| return 0; | ||
| } else { | ||
| return new Promise((resolve) => { | ||
| var timer; | ||
| if (ms >= 0) { | ||
| timer = setTimeout(() => { | ||
| delete wasmFSPollWaiters[key]; | ||
| resolve(0); | ||
| }, ms); | ||
| } | ||
| wasmFSPollWaiters[key] = {resolve, timer}; | ||
| }); | ||
| } | ||
| }, | ||
|
|
||
| _wasmfs_poll_notify__sig: 'vp', | ||
| _wasmfs_poll_notify: (key) => { | ||
| var entry = wasmFSPollWaiters[key]; | ||
| if (!entry) | ||
| return; | ||
|
|
||
| delete wasmFSPollWaiters[key]; | ||
| clearTimeout(entry.timer); | ||
| entry.resolve(1); | ||
| } |
There was a problem hiding this comment.
There is a critical race condition (lost wakeup) between _wasmfs_poll_wait and _wasmfs_poll_notify. When waitAsync is called on the main thread, it unlocks the C++ mutex before calling _wasmfs_poll_wait (which is necessary because it yields via Asyncify). If a worker thread calls notifyAll() and proxies _wasmfs_poll_notify during this brief window, _wasmfs_poll_notify will see that wasmFSPollWaiters[key] is not yet registered, and will return immediately without doing anything. Then, _wasmfs_poll_wait will run, register itself, and block indefinitely (or until timeout), missing the notification entirely.
To fix this, we can support a "pre-triggered" state in wasmFSPollWaiters. If _wasmfs_poll_notify is called before _wasmfs_poll_wait is registered, we record that the key was triggered. When _wasmfs_poll_wait is subsequently called, it will immediately consume the trigger and return 1 synchronously, avoiding the race condition and also avoiding an unnecessary Asyncify yield.
Additionally, both _wasmfs_poll_wait and _wasmfs_poll_notify use the $wasmFSPollWaiters library variable, so they must declare it in their __deps to ensure the compiler includes and handles it correctly.
_wasmfs_poll_wait__deps: ['$wasmFSPollWaiters'],
_wasmfs_poll_wait__async: 'auto',
_wasmfs_poll_wait__sig: 'ipi',
_wasmfs_poll_wait: (key, ms, sync) => {
if (sync) {
return 0;
}
var entry = wasmFSPollWaiters[key];
if (entry && entry.triggered) {
delete wasmFSPollWaiters[key];
return 1;
}
return new Promise((resolve) => {
var timer;
if (ms >= 0) {
timer = setTimeout(() => {
delete wasmFSPollWaiters[key];
resolve(0);
}, ms);
}
wasmFSPollWaiters[key] = {resolve, timer};
});
},
_wasmfs_poll_notify__deps: ['$wasmFSPollWaiters'],
_wasmfs_poll_notify__sig: 'vp',
_wasmfs_poll_notify: (key) => {
var entry = wasmFSPollWaiters[key];
if (!entry) {
wasmFSPollWaiters[key] = { triggered: true };
return;
}
if (entry.triggered) {
return;
}
delete wasmFSPollWaiters[key];
clearTimeout(entry.timer);
if (entry.resolve) {
entry.resolve(1);
}
}…d without PROXY_TO_PTHREAD
1bbcfe0 to
9eb2554
Compare
related to #26736