Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

provide makeMarshal and other vatPowers uniformly to all worker types #1776

Open
warner opened this issue Sep 16, 2020 · 3 comments
Open

provide makeMarshal and other vatPowers uniformly to all worker types #1776

warner opened this issue Sep 16, 2020 · 3 comments
Labels
bug Something isn't working SwingSet package: SwingSet

Comments

@warner
Copy link
Member

warner commented Sep 16, 2020

docs/static-vats.md defines the "vatPowers" that all vats receive. That document currently lists the following:

  • Remotable
  • getInterfaceOf
  • makeGetMeter (static+local vats only)
  • transformMetering (static+local vats only)
  • transformTildot

In addition, the code actually provides testLog (which appends strings to an array that unit tests can read), and I think I've seen evidence of makeMarshal being passed in under certain circumstances too.

However only the "local" vatManager currently receives all of these. Our other worker types don't. We need to provide uniform support for the same values everywhere.

Remotable and getInterfaceOf need to be shared with the same liveslots instance that the vat uses, since they close over an internal WeakMap to recognize pass-by-reference objects. In the cases where makeMarshal is provided, I think it might fall into the same category.

We could probably do without transformTildot (it's only used for the REPL), but for consistency we should probably implement it. In non-local workers, we must add a new worker-to-kernel protocol message. It will behave like a syscall (blocking, vat waits for kernel to respond). The vat worker will send a string of source code to the kernel, and the kernel will send a string of transformed source code back. This will keep Babel out of the worker process.

testLog can be implemented with a new one-way worker-to-kernel protocol message (it just takes a single string as argument), with no response. Alternatively, we might implement it by including an array of accumulated testLog strings in the deliveryResults message, but I'm not sure I like that approach.

We got one test to fail in a more-interesting way by providing a non-functional stub for testLog with the following diff:

diff --git a/packages/SwingSet/src/kernel/vatManager/nodeWorkerSupervisor.js b/packages/SwingSet/src/kernel/vatManager/nodeWorkerSupervisor.js
index 08cc1e91..723b6e02 100644
--- a/packages/SwingSet/src/kernel/vatManager/nodeWorkerSupervisor.js
+++ b/packages/SwingSet/src/kernel/vatManager/nodeWorkerSupervisor.js
@@ -103,7 +102,9 @@ parentPort.on('message', ([type, ...margs]) => {
       // vatPowers, but only if options tell us they're wanted. Maybe
       // transformTildot should be async and outsourced to the kernel
       // process/thread.
-      const vatPowers = { Remotable, getInterfaceOf };
+      const vatPowers = { Remotable, getInterfaceOf,
+                          testLog(_ignored) {}, // TODO: convey to kernel, append to testLog
+                        };
       dispatch = makeLiveSlots(
         syscall,
         state,
diff --git a/packages/SwingSet/src/kernel/vatManager/subprocessSupervisor.js b/packages/SwingSet/src/kernel/vatManager/subprocessSupervisor.js
index 74326c46..218f724b 100644
--- a/packages/SwingSet/src/kernel/vatManager/subprocessSupervisor.js
+++ b/packages/SwingSet/src/kernel/vatManager/subprocessSupervisor.js
@@ -118,7 +117,9 @@ fromParent.on('data', data => {
       // vatPowers, but only if options tell us they're wanted. Maybe
       // transformTildot should be async and outsourced to the kernel
       // process/thread.
-      const vatPowers = { Remotable, getInterfaceOf };
+      const vatPowers = { Remotable, getInterfaceOf,
+                          testLog(_ignored) {}, // TODO: convey to kernel, append to testLog
+                        };
       dispatch = makeLiveSlots(
         syscall,
         state,
diff --git a/packages/xs-vat-worker/src/vatWorker.js b/packages/xs-vat-worker/src/vatWorker.js
index cfdfc58a..8dfccc4e 100644
--- a/packages/xs-vat-worker/src/vatWorker.js
+++ b/packages/xs-vat-worker/src/vatWorker.js
@@ -124,7 +123,9 @@ function makeWorker(io, setImmediate) {
         // vatPowers, but only if options tell us they're wanted. Maybe
         // transformTildot should be async and outsourced to the kernel
         // process/thread.
-        const vatPowers = { Remotable, getInterfaceOf };
+        const vatPowers = { Remotable, getInterfaceOf,
+                            testLog(_ignored) {}, // TODO: convey to kernel, append to testLog
+                          };
         dispatch = makeLiveSlots(
           syscall,
           state,
@warner warner added bug Something isn't working SwingSet package: SwingSet labels Sep 16, 2020
warner added a commit that referenced this issue Sep 17, 2020
warner added a commit that referenced this issue Sep 17, 2020
This adds a new message to the vat-to-kernel protocol, `['testLog',
...args]`, to deliver the strings from the vat to the kernel. They get added
to the same `c.dump().log` array that local workers can write to. This is
solely for the benefit of unit tests.

refs #1776 (closing the `testLog` part, but not the other vatPowers)
warner added a commit that referenced this issue Sep 17, 2020
warner added a commit that referenced this issue Sep 17, 2020
This adds a new message to the vat-to-kernel protocol, `['testLog',
...args]`, to deliver the strings from the vat to the kernel. They get added
to the same `c.dump().log` array that local workers can write to. This is
solely for the benefit of unit tests.

refs #1776 (closing the `testLog` part, but not the other vatPowers)
warner added a commit that referenced this issue Sep 21, 2020
warner added a commit that referenced this issue Sep 21, 2020
This adds a new message to the vat-to-kernel protocol, `['testLog',
...args]`, to deliver the strings from the vat to the kernel. They get added
to the same `c.dump().log` array that local workers can write to. This is
solely for the benefit of unit tests.

refs #1776 (closing the `testLog` part, but not the other vatPowers)
@warner
Copy link
Member Author

warner commented Jan 27, 2021

@dckc this overlaps with code you were looking at this week. Not urgent, but wanted to circle back to the notion of a (non-transcript) syscall-like worker-to-kernel message to implement transformTildot

@dckc dckc changed the title provide testLog and other vatPowers uniformly to all worker types provide transformTildot and other vatPowers uniformly to all worker types Jan 27, 2021
@warner
Copy link
Member Author

warner commented Mar 29, 2021

This ought to be improved by the fixes for #2671 , although perhaps it won't go far enough. Ideally we'll have a single shared function that creates the vatPowers and liveslots instances for all worker types.

@dckc dckc removed their assignment Jul 22, 2021
@warner
Copy link
Member Author

warner commented Feb 18, 2022

All four worker types now provide makeMarshal and testLog to liveslots, and the rest are created by liveslots internally. We no longer provide transformTildot, transformMetering, or makeGetMeter. And Remotable and getInterfaceOf are imported by vats bundles internally.

I did notice that makeMarshal is provided to liveslots from import { makeMarshal } from '@endo/marshal' in three worker types, but there's a copy passed into allVatPowers for manager-local. I don't think this copy is used by liveslots, and it would be weird for the vat code to get a different copy than what liveslots is using. I think this is a leftover from when we needed to share an (impure) module instance between liveslots and the vat code (because it used a WeakMap to recognize Remotable/Far -marked objects), or maybe it has to do with stack trace censoring between the different consoles.

Anyways, I'm going to redefine this ticket to be about figuring out the makeMarshal confusion, and then we can close it.

@warner warner changed the title provide transformTildot and other vatPowers uniformly to all worker types provide makeMarshal and other vatPowers uniformly to all worker types Feb 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working SwingSet package: SwingSet
Projects
None yet
Development

No branches or pull requests

3 participants