node-worker/subprocess-node workers cannot do vatstore or baggage: remove? #4884
Assignees
Labels
Comments
Merged
warner
added a commit
that referenced
this issue
Jan 30, 2023
Most of our vat workers use `xs-worker`, which runs the code in an `xsnap` subprocess. A handful of vats (maybe just comms) run in a `local` worker, which is co-resident with the kernel. We have two additional worker types, `nodeWorker` (which uses threads, known as "workers" in Node.js), and `node-subprocess` (which runs a second copy of Node.js in a subprocess). I added these for completeness, and to keep us honest about handling different worker types. But they are incomplete (node-subprocess *could* block for a syscall, but I didn't implement that), underfeatured (nodeWorker *cannot* block for a syscall without heroics involving ShareArrayBuffers and Atomics), undertested (the previous lacking features prompted us to `test.skip` them), and unused by anything outside of our test suite. I have refactoring work in the pipeline that will be easier if there is less code to move around. So it's time for these to go. I might try to bring these back some day, but only when we have some interesting use case for them (which is more likely to be inside a browser, than under Node.js). closes #4884
warner
added a commit
that referenced
this issue
Jan 30, 2023
Most of our vat workers use `xs-worker`, which runs the code in an `xsnap` subprocess. A handful of vats (maybe just comms) run in a `local` worker, which is co-resident with the kernel. We have two additional worker types, `nodeWorker` (which uses threads, known as "workers" in Node.js), and `node-subprocess` (which runs a second copy of Node.js in a subprocess). I added these for completeness, and to keep us honest about handling different worker types. But they are incomplete (node-subprocess *could* block for a syscall, but I didn't implement that), underfeatured (nodeWorker *cannot* block for a syscall without heroics involving ShareArrayBuffers and Atomics), undertested (the previous lacking features prompted us to `test.skip` them), and unused by anything outside of our test suite. I have refactoring work in the pipeline that will be easier if there is less code to move around. So it's time for these to go. I might try to bring these back some day, but only when we have some interesting use case for them (which is more likely to be inside a browser, than under Node.js). closes #4884
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
What is the Problem Being Solved?
We nominally have four kinds of vat workers:
setup-based and non-transcripted vats)In a real deployment, we use
localfor the comms vat andsubprocess-xsnapfor everything else. In unit tests, we uselocalin a lot of cases where we need more visibility into what the vat is doing (we intentionally violate the isolation boundary, usually with asetup()-based vat).We keep the other two around to keep us honest and thinking about the future. The main feature that isn't available on these secondary worker types is the ability to do a blocking read of syscall results. This prevents
vatstoreGetand the device-invocation syscall from returning a result.To do this under Node.js threads will require some tricks (Atomics and a SharedArrayBuffer) that we've discussed but haven't tried to prototype yet. These are the same tricks we'd need to implement swingset inside a web browser, which is more likely to be a use case than anything involving Node.js threads. To do this under
subprocess-nodeis a Simple Matter Of Programming, with no special technology, we just haven't bothered to implement it yet.As @FUDCo ran into on #4873, the lack of
vatstoreGetprevents liveslots from initializing the "baggage", which forced him to disable the nodeworker/subprocess-node unit tests in that PR.So the question is: what should we do with these workers?
One relatively-low-effort task would be to implement the blocking read for subprocess-node, and then unskip the tests for that worker. This would need to replace the
on('data', handler)with something that does a blockingfs.read*call and waits for the full netstring to complete (ideally it would loop around a series of short "get as much as you can" reads until it has the whole length prefix, then do a single large blocking read of the expected length, just to minimize the number of reads and copies it does).A much higher-effort task would be to figure out the Atomics/SharedArrayBuffer trick to allow blocking reads across a thread boundary.
A super-low-effort step would be to abandon these two worker types entirely, and just delete/disable all the tests associated with them.
The text was updated successfully, but these errors were encountered: