Add emscripten_poll_with_callback and unify fd readiness on one poll wait-queue#27181
Add emscripten_poll_with_callback and unify fd readiness on one poll wait-queue#27181guybedford wants to merge 2 commits into
Conversation
c3ace69 to
658ecb9
Compare
…_callback Adds emscripten_poll_with_callback(fd, events, timeout, cb): a non-blocking single-fd poll that invokes cb(fd, revents) when the fd is ready or the timeout elapses. revents is passed by value. It does not suspend the caller, so it works without ASYNCIFY/JSPI. Returns -EBADF for a bad fd and -EPERM if the descriptor type can't deliver readiness callbacks (checked before arming, even when ready); closing an fd wakes its waiters with POLLNVAL. It is meant as an integration point for async runtimes and event loops that need to await I/O readiness without a blocking call or a stack switch: e.g. waiting for a socket to become readable/writable, or for an async-completion fd, and dispatching when ready. In ASYNCIFY/JSPI builds it complements blocking poll()/select(); in plain synchronous builds it is the only way to wait on an fd without spinning a poll loop. To support it, fd readiness now uses a single wait-queue on the file node, replacing three separate mechanisms (the socket event callbacks, the pipe readable handlers, and the blocking-poll notifier): - stream_ops.poll(stream) is now pure derivation; it no longer registers. - producers wake waiters via $notifyPollCallback(node, flags): SOCKFS.emit bridges socket events, PIPEFS writes wake the read end. - consumers register via $addPollCallback(node, cb): the async __syscall_poll registers one waiter per fd (re-deriving the set on wake), and emscripten_poll_with_callback registers a single-fd waiter. Sockets now feed the same seam, so blocking poll()/select() on a socket is woken by incoming data; previously sock_ops.poll() ignored the notifier. Tests: test_poll_callback (callback readiness, -EPERM/-EBADF gate, POLLNVAL close), test_poll_socket_blocking (blocking poll() woken by a delayed send; hangs before this change, passes after), and the core poll/ppoll/select/pipe blocking suites, including PROXY_TO_PTHREAD variants.
sbc100
left a comment
There was a problem hiding this comment.
I general I like the idea of exposing something like emscripten_poll_with_callback to userspace.
However there are several way to expose it:
- As callback-based API.
- As a promise-based API and returns promise_t to C/C++
- As a promise-based API that uses JSPI/asyncify.
Maybe more?
I think all the above use cases are valid, and I think it would be nice to expose them all, and ideally we would have a nice way to write just one of these and derive the rest of them.
We have been trying to come up with unified scheme to how to go about this for a while but so far its been kind of ad-hoc. This might be a good opportunity to define a use a policy creating now async-any-way-you-like function.
I think the ideal solution is that the JS library author writes a single async JS function in that most idiomatic way (i.e. async foo()) and then the native C/C++ developer should be able to automatically call that function in any of the above ways.
Having said all of that, it seems like this PR is really in two parts:
- And internal refactoring of how poll works.
- The exposing of a new poll-on-off function to userspace.
Would it be worth landing (1) while we figure out the best shape for (2) asyncronously?
| 'close': {{{ cDefs.POLLIN }}} | {{{ cDefs.POLLHUP }}}, | ||
| 'error': {{{ cDefs.POLLERR }}}, | ||
| }[event]; | ||
| if (flags) notifyPollCallback(FS.getStream(fd)?.node, flags); |
There was a problem hiding this comment.
Can flags ever be undefined here? i.e. are there events that are not lists in the flags mapping above?
Maybe assert(flags, "unhandled event .. ") instead?
| var sock = stream.node.sock; | ||
| // Wake any pending poll-callback waiters: the fd is going away (POLLNVAL), | ||
| // so they complete and release their keepalive rather than hang. | ||
| notifyPollCallback(stream.node, {{{ cDefs.POLLNVAL }}}); |
There was a problem hiding this comment.
How is this different the notifyPollCallback for close in the emit method above?
| } else { | ||
| flags = {{{ cDefs.POLLIN | cDefs.POLLOUT }}}; | ||
| } | ||
| flags = stream.stream_ops.poll ? stream.stream_ops.poll(stream) : ({{{ cDefs.POLLIN | cDefs.POLLOUT }}}); |
There was a problem hiding this comment.
Maybe expand this ternary for readability (closure compiler et al can always restore I think?)
| // Copy first: a woken waiter removes itself as it completes. | ||
| node?.pollCallbacks?.slice().forEach((cb) => cb(flags)); | ||
| }, | ||
| $addPollCallback: (node, cb) => { |
There was a problem hiding this comment.
Mark these new helpers as __internal ?
| // are entirely here. | ||
| $notifyPollCallback: (node, flags) => { | ||
| // Copy first: a woken waiter removes itself as it completes. | ||
| node?.pollCallbacks?.slice().forEach((cb) => cb(flags)); |
There was a problem hiding this comment.
Use for ... of over forEach ? IIRC our convention is to prefer for .. of loops
| #if PTHREADS || ASYNCIFY | ||
| pipe.notifyReadableHandlers(); | ||
| #endif | ||
| notifyPollCallback(pipe.readNode, {{{ cDefs.POLLRDNORM }}} | {{{ cDefs.POLLIN }}}); |
There was a problem hiding this comment.
I wonder if we can come up with a better name? Something like nodeStateChanged ? Or notifyNodeListeners?
Also, I wonder if this should be a method on the FS global? I guess its only needed for PIPFS and SOCKFS so maybe not a great idea?
| // readiness through $notifyPollCallback can be waited on. `pollAsync` is a | ||
| // flag, or a predicate when an instance (e.g. a listening socket) can't. | ||
| var pollAsync = stream.stream_ops.pollAsync; | ||
| if (typeof pollAsync == 'function') pollAsync = pollAsync(stream); |
There was a problem hiding this comment.
Can these two lines be written as just var pollAsync = stream.stream_ops.pollAsync?.(stream)
sbc100
left a comment
There was a problem hiding this comment.
Just to clear, I'm excited about the general direction here.
Regarding the specific shape the callback-based C APIs, I'm not sure about the timeout part. In general, I think the callback-based APIs we have today to not have timeouts but rather some kind of cancellation mechanism. It should be easy enough to then build your own timeout in userspace.
658ecb9 to
39c656a
Compare
Adds
emscripten_poll_with_callback, a non-blocking single-fd poll:cb(fd, revents)fires when the fd is ready or the timeout elapses;reventsis by value. It does not suspend the caller, so it works without ASYNCIFY/JSPI. Returns-EBADFfor a bad fd and-EPERMif the descriptor type can't deliver readiness callbacks (checked before arming, even when ready, likeepoll_ctl); closing an fd wakes its waiters withPOLLNVAL.eventsis a bitmask — register several conditions, fire once on whichever is ready first, re-arm to continue.It is meant as an integration point for async runtimes and event loops that need to await I/O readiness without a blocking call or a stack switch — e.g. waiting for a socket to become readable/writable and dispatching when ready. In ASYNCIFY/JSPI builds it complements blocking
poll()/select(); in plain synchronous builds it is the way to wait on an fd without spinning a poll loop. Unlike the global socket event callbacks, registration is per-fd and opt-in: you are only woken for the(fd, events)you armed, once.To support it, fd readiness now uses a single wait-queue on the file node, replacing three separate mechanisms (the socket event callbacks, the pipe readable handlers, and the blocking-poll notifier):
stream_ops.poll(stream)is now pure readiness derivation — it no longer registers notifications.$notifyPollCallback(node, flags):SOCKFS.emitbridges socket events,PIPEFSwrites wake the read end.$addPollCallback(node, cb): the async__syscall_pollregisters one waiter per fd and re-derives the set on any wake, andemscripten_poll_with_callbackregisters a single-fd waiter.A consequence of routing sockets through the same seam: blocking
poll()/select()on a socket is now woken by incoming data. Previouslysock_ops.poll()ignored the notifier, so a blockingpoll()on a socket could only time out.Tests:
test_poll_callback— callback readiness on a socket, the-EPERM/-EBADFcapability gate,POLLNVALon close.test_poll_socket_blocking— a blockingpoll()woken by a send that arrives only after it has blocked (sender thread under pthreads, timer under JSPI). It hangs on the pre-unification machinery and passes after, so it actually exercises the wake path.poll/ppoll/select/pipeblocking suites, includingPROXY_TO_PTHREAD.Size/perf: wasm unchanged; +~70 B JS on socket builds, none otherwise. No hot-path change beyond a short-circuiting notify on socket events / pipe writes.