Refactor async poll()/select() onto a per-inode readiness wait-queue#27226
Refactor async poll()/select() onto a per-inode readiness wait-queue#27226guybedford wants to merge 6 commits into
Conversation
Move the readiness wait-queue onto FSNode (addListener/notifyListeners) so dup'd fds share one queue, and rewrite the suspending poll() path (readPollfds/writePollfds/pollWait) on top of it. Producers (SOCKFS, PIPEFS) now notify the node on readiness transitions, and close() wakes waiters with POLLNVAL. The stream_ops.poll backend handler signature changes from poll(stream, timeout) to poll(stream) returning the current readiness mask.
guybedford
force-pushed
the
readiness-refactor
branch
from
f5b9a75 to
38d299d
Compare
sbc100
left a comment
sbc100
left a comment
There was a problem hiding this comment.
Great! Thanks for splitting this out.
lgtm with some comments
| // When proxied from a worker (PTHREADS) or able to suspend (ASYNCIFY/JSPI), | ||
| // block on the wait-queue: read the interests out of memory, wait, then | ||
| // write revents back into the (still-live) pollfd array and resolve. | ||
| if (isAsyncContext) { |
There was a problem hiding this comment.
I wonder if we should just skip pollWait here when timeout == 0? i.e. should this be if (isAsyncContext && timeout)? I guess that would be change compared the current behaviour so maybe shouldn't be part of this change ?
There was a problem hiding this comment.
Changing this would require changing the proxy runner to use Promise.resolve(return).then() instead of just calling rtn.then(...). Could work though.
| flags &= events | {{{ cDefs.POLLERR }}} | {{{ cDefs.POLLHUP }}} | {{{ cDefs.POLLNVAL }}}; | ||
| if (flags) count++; | ||
| {{{ makeSetValue('pollfd', C_STRUCTS.pollfd.revents, 'flags', 'i16') }}}; | ||
| if (timeout > 0) timer = setTimeout(() => finish(0), timeout); |
There was a problem hiding this comment.
Could also skip this if condition too.
There was a problem hiding this comment.
This one surprised me too - apparently negative timeouts are permitted to mean indefinite, so we do actually need to do this polarity check.
guybedford
force-pushed
the
readiness-refactor
branch
from
54603a4 to
6d38401
Compare
| return { | ||
| listeners, | ||
| entry | ||
| }; |
There was a problem hiding this comment.
Any reason not to make this (and the above inline object) a single line ? maybe its JS sytle to use multi-line in these cases?
There was a problem hiding this comment.
This is to match terser's beautify output I believe?
| // (not the fd) so dup'd fds share one queue. Only nodes that derive real | ||
| // readiness (sockets, pipes, and an epoll's own node) ever use this - | ||
| // always-ready types (regular files, ttys) never register or notify. | ||
| addListener(cb, exclusive = false) { |
There was a problem hiding this comment.
nit picking now, but this change does need the exclusive part.. maybe at least mention in the PR description that this change adds this even though its not technically using it? (Or leave to for the followup where its used?)
There was a problem hiding this comment.
Good call — kept the exclusive machinery but added a note in the PR description that it's groundwork for the EPOLLEXCLUSIVE path in the epoll followup (#27207) and is currently inert here (no caller passes exclusive).
2 participants
This refactors out a partial base from #27207.
Move the readiness wait-queue onto FSNode (addListener/notifyListeners) so dup'd fds share one queue, and rewrite the suspending poll() path (readPollfds/writePollfds/pollWait) on top of it. Producers (SOCKFS, PIPEFS) now notify the node on readiness transitions, and close() wakes waiters with POLLNVAL.
addListener(cb, exclusive)/notifyListeners(flags)— a per-inode Set of listener entries. It lives on the node (not the fd) so dup'd fds share one queue. Only nodes with real readiness (sockets, pipes) ever populate it; always-ready types (regular files, ttys) never touch it.poll()path is rewritten on top of it (readPollfds/writePollfds/pollWait/pollOne), replacing the old doPollAsync + makeNotifyCallback machinery.PIPEFSon writes,SOCKFSvia its emit bridge, andclose()wakes any waiter withPOLLNVAL.POLLRDHUP(only a fully closed connection isPOLLHUP), and a queued client makes a listening socketPOLLIN.The stream_ops.poll backend handler signature changes from
poll(stream, timeout)topoll(stream)returning the current readiness mask.Note: the
exclusiveparameter onaddListenerand the round-robin wakeup path innotifyListenersare deliberately layered in here as groundwork for the EPOLLEXCLUSIVE handling that lands in the epoll followup (#27207). No caller passesexclusivein this PR yet, so that branch is currently inert — it's included now to keep the wait-queue API stable across the split.