chore(swingset): refactor vatManagerFactory()

Refactor the creation of VatManagers to use a single
function (vatManagerFactory), which takes "managerOptions", which include
everything needed to configure the new manager.

The handling of options was tightened up, with precondition checks on the
options-bag contents.

One new managerOption is `managerType`, which specifies where we want the new
vat to run. Only `local` is supported right now (our usual Compartment
sharing the main thread approach), but #1299 / #1127 will introduce new types
that use separate threads, or subprocesses.

The controller/kernel acquired a new `shutdown()` method, to use in unit
tests. This is unused now, but once we add workers that spawn threads or
subprocesses, we'll need to call+await this at the end of any unit test that
creates thread/subprocess-based workers, otherwise the worker thread would
keep the process from ever exiting.

Author: warner

packages/SwingSet/src/assertOptions.js

@@ -0,0 +1,10 @@
+import { assert } from '@agoric/assert';
+
+export function assertKnownOptions(options, knownNames) {
+  assert(knownNames instanceof Array);
+  for (const name of Object.keys(options)) {
+    if (knownNames.indexOf(name) === -1) {
+      throw Error(`unknown option ${name}`);
+    }
+  }
+}

packages/SwingSet/src/controller.js

@@ -3,22 +3,21 @@
 import fs from 'fs';
 import path from 'path';
 import re2 from 're2';
-import { assert } from '@agoric/assert';
+import * as babelCore from '@babel/core';
+import * as babelParser from '@agoric/babel-parser';
+import babelGenerate from '@babel/generator';
+import anylogger from 'anylogger';
 
+import { assert } from '@agoric/assert';
 import { isTamed, tameMetering } from '@agoric/tame-metering';
 import bundleSource from '@agoric/bundle-source';
 import { importBundle } from '@agoric/import-bundle';
 import { initSwingStore } from '@agoric/swing-store-simple';
 import { HandledPromise } from '@agoric/eventual-send';
-
 import { makeMeteringTransformer } from '@agoric/transform-metering';
-import * as babelCore from '@babel/core';
 import { makeTransform } from '@agoric/transform-eventual-send';
-import * as babelParser from '@agoric/babel-parser';
-import babelGenerate from '@babel/generator';
-
-import anylogger from 'anylogger';
 
+import { assertKnownOptions } from './assertOptions';
 import { waitUntilQuiescent } from './waitUntilQuiescent';
 import { insistStorageAPI } from './storageAPI';
 import { insistCapData } from './capdata';
@@ -53,6 +52,13 @@ function byName(a, b) {
   return 0;
 }
 
+const KNOWN_CREATION_OPTIONS = harden([
+  'enablePipelining',
+  'metered',
+  'enableSetup',
+  'managerType',
+]);
+
 /**
  * Scan a directory for files defining the vats to bootstrap for a swingset, and
  * produce a swingset config object for what was found there.  Looks for files
@@ -246,7 +252,15 @@ export async function buildVatController(
   // two vattps, must handle somehow.
   const commsVatSourcePath = require.resolve('./vats/comms');
   const commsVatBundle = await bundleSource(commsVatSourcePath);
-  kernel.addGenesisVat('comms', commsVatBundle, {}, { enablePipelining: true }); // todo: allowSetup
+  kernel.addGenesisVat(
+    'comms',
+    commsVatBundle,
+    {},
+    {
+      enablePipelining: true,
+      enableSetup: true,
+    },
+  );
 
   // vat-tp is added automatically, but TODO: bootstraps must still connect
   // it to comms
@@ -260,7 +274,7 @@ export async function buildVatController(
   const timerWrapperBundle = await bundleSource(timerWrapperSourcePath);
   kernel.addGenesisVat('timer', timerWrapperBundle);
 
-  async function addGenesisVat(
+  function addGenesisVat(
     name,
     bundleName,
     vatParameters = {},
@@ -370,19 +384,13 @@ export async function buildVatController(
   }
 
   if (config.vats) {
-    const vats = [];
     for (const name of Object.keys(config.vats)) {
-      const v = config.vats[name];
-      vats.push(
-        addGenesisVat(
-          name,
-          v.bundleName || name,
-          v.parameters || {},
-          v.creationOptions || {},
-        ),
-      );
+      const { bundleName, parameters, creationOptions = {} } = config.vats[
+        name
+      ];
+      assertKnownOptions(creationOptions, KNOWN_CREATION_OPTIONS);
+      addGenesisVat(name, bundleName || name, parameters, creationOptions);
     }
-    await Promise.all(vats);
   }
 
   // start() may queue bootstrap if state doesn't say we did it already. It
@@ -413,6 +421,10 @@ export async function buildVatController(
       return kernel.step();
     },
 
+    async shutdown() {
+      return kernel.shutdown();
+    },
+
     getStats() {
       return JSON.parse(JSON.stringify(kernel.getStats()));
     },

packages/SwingSet/src/kernel/dynamicVat.js

@@ -1,3 +1,4 @@
+import { assertKnownOptions } from '../assertOptions';
 import { makeVatSlot } from '../parseVatSlots';
 
 export function makeVatRootObjectSlot() {
@@ -35,17 +36,9 @@ export function makeDynamicVatCreator(stuff) {
    * citing this vatID
    */
 
-  function createVatDynamically(vatSourceBundle, options = {}) {
-    const {
-      metered = true,
-      creationOptions = {},
-      vatParameters = {},
-      ...unknownOptions
-    } = options;
-    if (Object.keys(unknownOptions).length) {
-      const msg = JSON.stringify(Object.keys(unknownOptions));
-      throw Error(`createVatDynamically got unknown options ${msg}`);
-    }
+  function createVatDynamically(vatSourceBundle, dynamicOptions = {}) {
+    assertKnownOptions(dynamicOptions, ['metered', 'vatParameters']);
+    const { metered = true, vatParameters = {} } = dynamicOptions;
 
     const vatID = allocateUnusedVatID();
     let terminated = false;
@@ -77,28 +70,23 @@ export function makeDynamicVatCreator(stuff) {
       );
     }
 
-    const managerOptions = {
-      metered,
-      notifyTermination: metered ? notifyTermination : undefined,
-      vatPowerType: 'dynamic',
-      creationOptions,
-      vatParameters,
-    };
-
     async function build() {
       if (typeof vatSourceBundle !== 'object') {
         throw Error(
           `createVatDynamically() requires bundle, not a plain string`,
         );
       }
 
-      const manager = await vatManagerFactory.createFromBundle(
-        vatSourceBundle,
-        vatID,
-        managerOptions,
-      );
-      const addOptions = {}; // enablePipelining:false
-      addVatManager(vatID, manager, addOptions);
+      const managerOptions = {
+        bundle: vatSourceBundle,
+        metered,
+        enableSetup: false,
+        enableInternalMetering: false,
+        notifyTermination: metered ? notifyTermination : undefined,
+        vatParameters,
+      };
+      const manager = await vatManagerFactory(vatID, managerOptions);
+      addVatManager(vatID, manager, managerOptions);
     }
 
     function makeSuccessResponse() {

packages/SwingSet/src/kernel/kernel.js

@@ -3,7 +3,8 @@
 import { makeMarshal, Remotable, getInterfaceOf } from '@agoric/marshal';
 import { assert, details } from '@agoric/assert';
 import { importBundle } from '@agoric/import-bundle';
-import { makeVatManagerFactory } from './vatManager/vatManager';
+import { assertKnownOptions } from '../assertOptions';
+import { makeVatManagerFactory } from './vatManager/factory';
 import makeDeviceManager from './deviceManager';
 import { wrapStorage } from './state/storageWrapper';
 import makeKernelKeeper from './state/kernelKeeper';
@@ -82,16 +83,21 @@ export default function buildKernel(kernelEndowments) {
   // in the kernel table, promises and resolvers are both indexed by the same
   // value. kernelPromises[promiseID] = { decider, subscribers }
 
-  // The 'staticVatPowers' are given to vats as arguments of setup().
-  // Liveslots provides them, and more, as the only argument to
-  // buildRootObject(). They represent controlled authorities that come from
-  // the kernel but that do not go through the syscall mechanism (so they
-  // aren't included in the replay transcript), so they must not be
-  // particularly stateful. If any of them behave differently from one
-  // invocation to the next, the vat code which uses it will not be a
-  // deterministic function of the transcript, breaking our
-  // orthogonal-persistence model. They can have access to state, but they
-  // must not let it influence the data they return to the vat.
+  // The "Vat Powers" are given to vats as arguments of setup(). Liveslots
+  // provides them, and more, as the only argument to buildRootObject(). They
+  // represent controlled authorities that come from the kernel but that do
+  // not go through the syscall mechanism (so they aren't included in the
+  // replay transcript), so they must not be particularly stateful. If any of
+  // them behave differently from one invocation to the next, the vat code
+  // which uses it will not be a deterministic function of the transcript,
+  // breaking our orthogonal-persistence model. They can have access to
+  // state, but they must not let it influence the data they return to the
+  // vat.
+
+  // Not all vats get all powers. We're phasing out in-vat metering, so
+  // `makeGetMeter` and `transformMetering` are only available to static vats
+  // in a local worker, and will eventually go away entirely once Spawner
+  // uses dynamic vats.
 
   // These will eventually be provided by the in-worker supervisor instead.
 
@@ -101,9 +107,9 @@ export default function buildKernel(kernelEndowments) {
   // maybe transformMetering) are imported by the vat, not passed in an
   // argument. The powerful one (makeGetMeter) should only be given to the
   // root object, to share with (or withhold from) other objects as it sees
-  // fit. TODO: makeGetMeter and transformMetering will go away once #1288
-  // lands and zoe no longer needs to do metering within a vat.
-  const staticVatPowers = harden({
+  // fit. TODO: makeGetMeter and transformMetering will go away
+
+  const allVatPowers = harden({
     Remotable,
     getInterfaceOf,
     makeGetMeter: meterManager.makeGetMeter,
@@ -114,14 +120,6 @@ export default function buildKernel(kernelEndowments) {
     testLog,
   });
 
-  // dynamic vats don't get control over their own metering, nor testLog
-  const dynamicVatPowers = harden({
-    Remotable,
-    getInterfaceOf,
-    transformTildot: (...args) =>
-      meterManager.runWithoutGlobalMeter(transformTildot, ...args),
-  });
-
   function vatNameToID(name) {
     const vatID = kernelKeeper.getVatIDForName(name);
     insistVatID(vatID);
@@ -405,21 +403,20 @@ export default function buildKernel(kernelEndowments) {
     if (typeof setup !== 'function') {
       throw Error(`setup is not a function, rather ${setup}`);
     }
-    const knownCreationOptions = new Set(['enablePipelining', 'metered']);
-    for (const k of Object.keys(creationOptions)) {
-      if (!knownCreationOptions.has(k)) {
-        throw Error(`unknown option ${k}`);
-      }
-    }
-
+    assertKnownOptions(creationOptions, ['enablePipelining', 'metered']);
     if (started) {
       throw Error(`addGenesisVat() cannot be called after kernel.start`);
     }
     if (genesisVats.has(name)) {
       throw Error(`vatID ${name} already added`);
     }
-
-    genesisVats.set(name, { setup, vatParameters, creationOptions });
+    const managerOptions = {
+      ...creationOptions,
+      setup,
+      enableSetup: true,
+      vatParameters,
+    };
+    genesisVats.set(name, managerOptions);
   }
 
   function addGenesisVat(
@@ -434,19 +431,33 @@ export default function buildKernel(kernelEndowments) {
     if (typeof bundle !== 'object') {
       throw Error(`bundle is not an object, rather ${bundle}`);
     }
-    const knownCreationOptions = new Set(['enablePipelining', 'metered']);
-    for (const k of Object.getOwnPropertyNames(creationOptions)) {
-      if (!knownCreationOptions.has(k)) {
-        throw Error(`unknown option ${k}`);
-      }
-    }
+    assertKnownOptions(creationOptions, [
+      'enablePipelining',
+      'metered',
+      'managerType',
+      'enableSetup',
+      'enableInternalMetering',
+    ]);
     if (started) {
       throw Error(`addGenesisVat() cannot be called after kernel.start`);
     }
     if (genesisVats.has(name)) {
       throw Error(`vatID ${name} already added`);
     }
-    genesisVats.set(name, { bundle, vatParameters, creationOptions });
+    // TODO: We need to support within-vat metering (for the Spawner) until
+    // #1343 is fixed, after which we can remove
+    // managerOptions.enableInternalMetering . For now, it needs to be
+    // enabled for our internal unit test (which could easily add this to its
+    // config object) and for the spawner vat (not so easy). To avoid deeper
+    // changes, we enable it for *all* static vats here. Once #1343 is fixed,
+    // remove this addition and all support for internal metering.
+    const managerOptions = {
+      ...creationOptions,
+      bundle,
+      enableInternalMetering: true,
+      vatParameters,
+    };
+    genesisVats.set(name, managerOptions);
   }
 
   function addGenesisDevice(name, bundle, endowments) {
@@ -547,11 +558,10 @@ export default function buildKernel(kernelEndowments) {
   }
 
   const vatManagerFactory = makeVatManagerFactory({
-    dynamicVatPowers,
+    allVatPowers,
     kernelKeeper,
     makeVatEndowments,
     meterManager,
-    staticVatPowers,
     testLog,
     transformMetering,
     waitUntilQuiescent,
@@ -565,13 +575,13 @@ export default function buildKernel(kernelEndowments) {
    * might tell the manager to replay the transcript later, if it notices
    * we're reloading a saved state vector.
    */
-  function addVatManager(vatID, manager, creationOptions) {
+  function addVatManager(vatID, manager, managerOptions) {
     // addVatManager takes a manager, not a promise for one
     assert(
       manager.deliver && manager.setVatSyscallHandler,
       `manager lacks .deliver, isPromise=${manager instanceof Promise}`,
     );
-    const { enablePipelining = false } = creationOptions;
+    const { enablePipelining = false } = managerOptions;
     // This should create the vatKeeper. Other users get it from the
     // kernelKeeper, so we don't need a reference ourselves.
     kernelKeeper.allocateVatKeeperIfNeeded(vatID);
@@ -715,22 +725,12 @@ export default function buildKernel(kernelEndowments) {
 
     // instantiate all vats
     for (const name of genesisVats.keys()) {
-      const { setup, bundle, vatParameters, creationOptions } = genesisVats.get(
-        name,
-      );
+      const managerOptions = genesisVats.get(name);
       const vatID = kernelKeeper.allocateVatIDForNameIfNeeded(name);
       console.debug(`Assigned VatID ${vatID} for genesis vat ${name}`);
       // eslint-disable-next-line no-await-in-loop
-      const manager = await (setup
-        ? vatManagerFactory.createFromSetup(setup, vatID)
-        : vatManagerFactory.createFromBundle(bundle, vatID, {
-            metered: false,
-            vatPowerType: 'static',
-            allowSetup: true, // TODO: only needed by comms, disallow elsewhere
-            vatParameters,
-            creationOptions,
-          }));
-      addVatManager(vatID, manager, creationOptions);
+      const manager = await vatManagerFactory(vatID, managerOptions);
+      addVatManager(vatID, manager, managerOptions);
     }
 
     function createVatDynamicallyByName(bundleName, options) {
@@ -854,6 +854,12 @@ export default function buildKernel(kernelEndowments) {
     return count;
   }
 
+  // mostly used by tests, only needed with thread/process-based workers
+  function shutdown() {
+    const vatRecs = Array.from(ephemeral.vats.values());
+    return Promise.all(vatRecs.map(rec => rec.manager.shutdown()));
+  }
+
   const kernel = harden({
     // these are meant for the controller
     addGenesisVat,
@@ -867,6 +873,8 @@ export default function buildKernel(kernelEndowments) {
     step,
     run,
 
+    shutdown,
+
     // the rest are for testing and debugging
 
     addGenesisVatSetup,