Repository navigation
Surface IPC end-of-stream on win32 - #92
bavulapati wants to merge 1 commit into
Conversation
The win32 IPC layer had no EOF path: bare_ipc_read never returned end-of-stream and bare_ipc__on_read asserted nread >= 0, so a worklet closing its IPC on exit could not be observed by the host (and risked aborting on UV_EOF). Mark EOF in on_read, notify the readable poll, and return a zero-length read from bare_ipc_read. Flush the worklet loop after run so the close completes on Windows, and make the EOF tests poll-based so they run on all platforms. Closes #91.
| // and the host sees EOF before we park waiting for teardown. This is a no-op | ||
| // where uv_close is synchronous (posix) but is required on platforms where | ||
| // the close only completes via the loop (win32). | ||
| uv_run(&loop, UV_RUN_NOWAIT); |
There was a problem hiding this comment.
Ah, dang; ipc.destroy() is of course asynchronous and scheduling additional asynchronous work from an exit handler is strictly forbidden and leads to issues like this. We need to do something different.
|
Agreed the root cause is closing The graceful path has a clean fix that keeps the close inside the live loop: move it from Bare.on('suspend', () => ipc.unref())
.on('resume', () => ipc.ref())
.on('beforeExit', () => ipc.destroy()) // was 'exit'That covers loop-drain exit and the host-initiated The one case it does not cover is a worklet that calls I looked for a way to close that gap in bare-kit without reintroducing async-from-exit or a pump, and did not find a clean one:
So the residual question is only about the
Happy to implement whichever shape you prefer. I will close holepunchto/bare#160 in favor of the bare-kit fix. |
|
The crux of the issue seems to be that the ownership is muddy. Bare Kit should really just |
|
Picking up your point about ownership, I sketched where it leads so we can shape it together before committing to code. The direction: native allocates and owns the IPC descriptors instead of JS One design choice I'd like your take on: native being the sole closer means JS can't hold a Where I'd really like to pair is the platform split. posix looks straightforward: native holds the raw fd, writes with A way we could split it, if it suits you: I take the shared + JS + posix end and get the exit-EOF suite green on darwin, and we sort out the win32 write path between us (however you'd like to divide that). Worth flagging that posix already reaches EOF today, so that half is mostly a refactor that removes the forbidden pattern; the behavior that actually goes green is win32. One caveat that holds regardless of how we shape it: on a hard If this tracks for you, I'll get started on the posix end and open a PR we can iterate on. I'll also close holepunchto/bare#160. |
|
I had blissfully forgotten about the whole Windows socket debacle 🤦 That's not a path we want to go down, it only leads to pain. We probably need to accept that we're using the wrong tool for the job. IPC over pipes and sockets works really well when you couple if with |
|
Agreed, in-process queue is the right call; it turns the hard part into just signalling. On the primitive: bare-channel assumes both ends are JS (its port needs a I would pull it into bare-kit as a proper fetched dependency rather than copying loose files, which needs a small |
|
No external dependencies for this, it's simple enough for the entire implementation to live in Bare Kit. |
|
Superseded by #94, which fixes this on the queue transport rather than patching EOF onto the pipes. Closing. |
Bare Kit is in-process (worklets are threads), so the pipe-based IPC with its async close-on-exit was the wrong tool and the root of the EOF and win32 problems (holepunchto/bare#160, #92). Move the transport onto the in-process queue primitive: - Bare.IPC becomes a bare-stream Duplex over native queue ops that worklet.c injects (read/write/close/ref/unref); end() closes the queue so the host reaches EOF. - The host bare_ipc runs over the queue's thread port; the apple poll is a manual dispatch source fired by the queue signal, reporting readiness via bare_queue_readable/writable. - The worklet closes its queue end synchronously after bare_run, so the host reaches EOF on every exit path (loop drain, Bare.exit, IPC.end, terminate) with no loop pump and no async scheduled from exit. - bare_worklet_destroy joins the worklet thread (terminating and waking it first) so the runtime and queue finish tearing down before the host returns. - Delete the pipe posix/ipc.c. Darwin only so far: android/linux/win32 polls are not yet ported. All 14 darwin worklet and queue tests pass.
Bare Kit is in-process - worklets are threads - so the pipe transport, with its async close on exit, was the wrong tool and the root of the EOF and win32 problems (holepunchto/bare#160, #92). Bare.IPC becomes a bare-stream Duplex over the queue, and the worklet closes its end synchronously after bare_run, so the host reaches EOF on every exit path with no loop pump and nothing scheduled from the exit event. The host polls readiness rather than reading a pipe: apple keeps its dispatch source, linux and win32 now share one uv implementation instead of a hand-rolled epoll thread and a pipe loop, and android keeps its looper with the ipc owning the eventfd. bare_ipc_get_incoming and bare_ipc_get_outgoing go with the pipes; the win32 exports follow. The two EOF tests now run on win32, which is what this was for.
Bare Kit is in-process - worklets are threads - so the pipe transport, with its async close on exit, was the wrong tool and the root of the EOF and win32 problems (holepunchto/bare#160, #92). Bare.IPC becomes a bare-stream Duplex over the queue, and the worklet closes its end synchronously after bare_run, so the host reaches EOF on every exit path with no loop pump and nothing scheduled from the exit event. The host polls readiness rather than reading a pipe: apple keeps its dispatch source, linux and win32 now share one uv implementation instead of a hand-rolled epoll thread and a pipe loop, and android keeps its looper with the ipc owning the eventfd. The two poll tests closed their loop while their poll was still armed. Pipes hid that: their EOF arrived asynchronously at thread teardown, while the queue closes synchronously, so the wake lands in the window and the callback posts to a closed loop - a segfault on linux-x64. The poll is retired first now. bare_ipc_get_incoming and bare_ipc_get_outgoing go with the pipes; the win32 exports follow. Both EOF tests now run on win32, which is what this was for.
Bare Kit is in-process - worklets are threads - so the pipe transport, with its async close on exit, was the wrong tool and the root of the EOF and win32 problems (holepunchto/bare#160, #92). Bare.IPC becomes a bare-stream Duplex over the queue, and the worklet closes its end synchronously after bare_run, so the host reaches EOF on every exit path with no loop pump and nothing scheduled from the exit event. The host polls readiness rather than reading a pipe: apple keeps its dispatch source, linux and win32 now share one uv implementation instead of a hand-rolled epoll thread and a pipe loop, and android keeps its looper with the ipc owning the eventfd. The two poll tests closed their loop while their poll was still armed. Pipes hid that: their EOF arrived asynchronously at thread teardown, while the queue closes synchronously, so the wake lands in the window and the callback posts to a closed loop - a segfault on linux-x64. The poll is retired first now. bare_ipc_get_incoming and bare_ipc_get_outgoing go with the pipes; the win32 exports follow. Both EOF tests now run on win32, which is what this was for.
2 participants
Follow-up to #88 / #83, closing the win32 gap tracked in #91.
#88 makes a worklet close
Bare.IPCon exit so the host reads EOF and can detect a stopped worklet. That worked on the posix IPC path but not win32, whose IPC layer had no end-of-stream support:bare_ipc_readonly drained a buffer (never returned EOF), andbare_ipc__on_readassertednread >= 0, so a realUV_EOFwould abort. #88 scoped the EOF tests off win32 as a stopgap.This adds the missing EOF path:
bare_ipc__on_read: onUV_EOF, mark aneofflag, stop reading, and fire the readable poll so the consumer gets notified.bare_ipc_read: return a zero-length read (len 0) once EOF is reached and the buffer is drained.worklet.c: flush the loop (uv_run(&loop, UV_RUN_NOWAIT)) afterbare_runreturns - a no-op whereuv_closeis synchronous (posix), but required on win32 so the worklet's own IPC close completes before it parks.worklet-exit-eof/worklet-jsend-eofto be poll-based (the portable read model) and re-enabled them on win32.Validation: posix locally (darwin, full suite 13/13, EOF tests 5/5 stable); win32 via the CI
Test / win32-*jobs, which now run the re-enabled EOF tests on real Windows.The win32 IPC layer is yours by design - happy to adjust the shape (e.g. how EOF is surfaced through the poll) if you'd prefer it done differently.