From afeced85a7f3523aed3655c3d498ecdc9314ef02 Mon Sep 17 00:00:00 2001 From: Brian Warner Date: Wed, 16 Sep 2020 12:46:31 -0700 Subject: [PATCH 1/5] fix: clean up worker subprocess spawning fixes #1777 --- packages/SwingSet/src/controller.js | 22 ++++++++++++++----- .../SwingSet/src/spawnSubprocessWorker.js | 10 ++------- 2 files changed, 18 insertions(+), 14 deletions(-) diff --git a/packages/SwingSet/src/controller.js b/packages/SwingSet/src/controller.js index e6ed646780e..ad6418803dd 100644 --- a/packages/SwingSet/src/controller.js +++ b/packages/SwingSet/src/controller.js @@ -2,6 +2,7 @@ import fs from 'fs'; import path from 'path'; +import process from 'process'; import re2 from 're2'; import { Worker } from 'worker_threads'; import * as babelCore from '@babel/core'; @@ -353,6 +354,7 @@ export async function buildVatController( }`, ); + // this launches a worker in a Node.js thread (aka "Worker") function makeNodeWorker() { // TODO: after we move away from `-r esm` and use real ES6 modules, point // this at nodeWorkerSupervisor.js instead of the CJS intermediate @@ -362,17 +364,25 @@ export async function buildVatController( return new Worker(supercode); } + // launch a worker in a subprocess (which runs Node.js) + function startSubprocessWorkerNode() { + const supercode = require.resolve( + './kernel/vatManager/subprocessSupervisor.js', + ); + return startSubprocessWorker(process.execPath, ['-r', 'esm', supercode]); + } + + let startSubprocessWorkerXS; + const xsWorkerBin = locateWorkerBin({ resolve: path.resolve }); + if (fs.existsSync(xsWorkerBin)) { + startSubprocessWorkerXS = () => startSubprocessWorker(xsWorkerBin); + } + function writeSlogObject(_obj) { // TODO sqlite // console.log(`--slog ${JSON.stringify(obj)}`); } - const startSubprocessWorkerNode = () => startSubprocessWorker(); - const xsWorkerBin = locateWorkerBin({ resolve: path.resolve }); - const startSubprocessWorkerXS = fs.existsSync(xsWorkerBin) - ? () => startSubprocessWorker({ execPath: xsWorkerBin, args: [] }) - : undefined; - const kernelEndowments = { waitUntilQuiescent, hostStorage, diff --git a/packages/SwingSet/src/spawnSubprocessWorker.js b/packages/SwingSet/src/spawnSubprocessWorker.js index dbe5af44616..e02008d6225 100644 --- a/packages/SwingSet/src/spawnSubprocessWorker.js +++ b/packages/SwingSet/src/spawnSubprocessWorker.js @@ -1,5 +1,4 @@ // this file is loaded by the controller, in the start compartment -import process from 'process'; import { spawn } from 'child_process'; import Netstring from 'netstring-stream'; @@ -10,19 +9,14 @@ function parentLog(first, ...args) { // console.error(`--parent: ${first}`, ...args); } -const supercode = require.resolve( - './kernel/vatManager/subprocessSupervisor.js', -); // we send on fd3, and receive on fd4. We pass fd1/2 (stdout/err) through, so // console log/err from the child shows up normally. We don't use Node's // built-in serialization feature ('ipc') because the child process won't // always be Node. const stdio = harden(['inherit', 'inherit', 'inherit', 'pipe', 'pipe']); -export function startSubprocessWorker(options) { - const execPath = options.execPath || process.execPath; - const args = options.args || ['-r', 'esm', supercode]; - const proc = spawn(execPath, args, { stdio }); +export function startSubprocessWorker(execPath, procArgs = []) { + const proc = spawn(execPath, procArgs, { stdio }); const toChild = Netstring.writeStream(); toChild.pipe(proc.stdio[3]); From 9e6e31ac55521893b6fdf31785bb901345ed46af Mon Sep 17 00:00:00 2001 From: Brian Warner Date: Wed, 16 Sep 2020 12:28:39 -0700 Subject: [PATCH 2/5] fix: handle syscallResult and deliveryResult consistently among workers fixes #1775 --- .../src/kernel/vatManager/nodeWorker.js | 9 ++++++- .../kernel/vatManager/nodeWorkerSupervisor.js | 22 +++++++++------- .../kernel/vatManager/subprocessSupervisor.js | 22 +++++++++------- .../vatManager/worker-subprocess-node.js | 11 +++++++- packages/xs-vat-worker/src/vatWorker.js | 26 +++++++++++-------- 5 files changed, 57 insertions(+), 33 deletions(-) diff --git a/packages/SwingSet/src/kernel/vatManager/nodeWorker.js b/packages/SwingSet/src/kernel/vatManager/nodeWorker.js index 7e249102cd1..2f631d1dd5c 100644 --- a/packages/SwingSet/src/kernel/vatManager/nodeWorker.js +++ b/packages/SwingSet/src/kernel/vatManager/nodeWorker.js @@ -55,10 +55,16 @@ export function makeNodeWorkerVatManagerFactory(tools) { transcriptManager, ); function handleSyscall(vatSyscallObject) { + // we are invoked by an async postMessage from the worker thread, whose + // vat code has moved on (it really wants a synchronous/immediate + // syscall) const type = vatSyscallObject[0]; if (type === 'callNow') { throw Error(`nodeWorker cannot block, cannot use syscall.callNow`); } + // This might throw an Error if the syscall was faulty, in which case + // the vat will be terminated soon. It returns a vatSyscallResults, + // which we discard because there is nobody to send it to. doSyscall(vatSyscallObject); } @@ -96,7 +102,8 @@ export function makeNodeWorkerVatManagerFactory(tools) { if (waiting) { const resolve = waiting; waiting = null; - resolve(); + const deliveryResult = args; + resolve(deliveryResult); } } else { parentLog(`unrecognized uplink message ${type}`); diff --git a/packages/SwingSet/src/kernel/vatManager/nodeWorkerSupervisor.js b/packages/SwingSet/src/kernel/vatManager/nodeWorkerSupervisor.js index 705d78fe11d..b1e9955ca1b 100644 --- a/packages/SwingSet/src/kernel/vatManager/nodeWorkerSupervisor.js +++ b/packages/SwingSet/src/kernel/vatManager/nodeWorkerSupervisor.js @@ -46,6 +46,16 @@ async function doProcess(dispatchRecord, errmsg) { workerLog(`runAndWait`); await runAndWait(() => dispatch[dispatchOp](...dispatchArgs), errmsg); workerLog(`doProcess done`); + const vatDeliveryResults = harden(['ok']); + return vatDeliveryResults; +} + +function doMessage(targetSlot, msg) { + const errmsg = `vat[${targetSlot}].${msg.method} dispatch failed`; + return doProcess( + ['deliver', targetSlot, msg.method, msg.args, msg.result], + errmsg, + ); } function doNotify(vpid, vp) { @@ -65,7 +75,6 @@ function doNotify(vpid, vp) { } } -let syscallLog; parentPort.on('message', ([type, ...margs]) => { workerLog(`received`, type); if (type === 'start') { @@ -121,16 +130,9 @@ parentPort.on('message', ([type, ...margs]) => { } const [dtype, ...dargs] = margs; if (dtype === 'message') { - const [targetSlot, msg] = dargs; - const errmsg = `vat[${targetSlot}].${msg.method} dispatch failed`; - doProcess( - ['deliver', targetSlot, msg.method, msg.args, msg.result], - errmsg, - ).then(() => { - sendUplink(['deliverDone']); - }); + doMessage(...dargs).then(res => sendUplink(['deliverDone', ...res])); } else if (dtype === 'notify') { - doNotify(...dargs).then(() => sendUplink(['deliverDone', syscallLog])); + doNotify(...dargs).then(res => sendUplink(['deliverDone', ...res])); } else { throw Error(`bad delivery type ${dtype}`); } diff --git a/packages/SwingSet/src/kernel/vatManager/subprocessSupervisor.js b/packages/SwingSet/src/kernel/vatManager/subprocessSupervisor.js index 52c65209ab9..5a8c71abc85 100644 --- a/packages/SwingSet/src/kernel/vatManager/subprocessSupervisor.js +++ b/packages/SwingSet/src/kernel/vatManager/subprocessSupervisor.js @@ -42,6 +42,16 @@ async function doProcess(dispatchRecord, errmsg) { workerLog(`runAndWait`); await runAndWait(() => dispatch[dispatchOp](...dispatchArgs), errmsg); workerLog(`doProcess done`); + const vatDeliveryResults = harden(['ok']); + return vatDeliveryResults; +} + +function doMessage(targetSlot, msg) { + const errmsg = `vat[${targetSlot}].${msg.method} dispatch failed`; + return doProcess( + ['deliver', targetSlot, msg.method, msg.args, msg.result], + errmsg, + ); } function doNotify(vpid, vp) { @@ -79,7 +89,6 @@ function sendUplink(msg) { // toParent.write('child ack'); // }); -let syscallLog; fromParent.on('data', data => { const [type, ...margs] = JSON.parse(data); workerLog(`received`, type); @@ -136,16 +145,9 @@ fromParent.on('data', data => { } const [dtype, ...dargs] = margs; if (dtype === 'message') { - const [targetSlot, msg] = dargs; - const errmsg = `vat[${targetSlot}].${msg.method} dispatch failed`; - doProcess( - ['deliver', targetSlot, msg.method, msg.args, msg.result], - errmsg, - ).then(() => { - sendUplink(['deliverDone']); - }); + doMessage(...dargs).then(res => sendUplink(['deliverDone', ...res])); } else if (dtype === 'notify') { - doNotify(...dargs).then(() => sendUplink(['deliverDone', syscallLog])); + doNotify(...dargs).then(res => sendUplink(['deliverDone', ...res])); } else { throw Error(`bad delivery type ${dtype}`); } diff --git a/packages/SwingSet/src/kernel/vatManager/worker-subprocess-node.js b/packages/SwingSet/src/kernel/vatManager/worker-subprocess-node.js index 056e8e05f55..90c4ebcf68d 100644 --- a/packages/SwingSet/src/kernel/vatManager/worker-subprocess-node.js +++ b/packages/SwingSet/src/kernel/vatManager/worker-subprocess-node.js @@ -56,10 +56,18 @@ export function makeNodeSubprocessFactory(tools) { transcriptManager, ); function handleSyscall(vatSyscallObject) { + // We are currently invoked by an async piped from the worker thread, + // whose vat code has moved on (it really wants a synchronous/immediate + // syscall). TODO: unlike threads, subprocesses could be made to wait + // by doing a blocking read from the pipe, so we could fix this, and + // re-enable syscall.callNow const type = vatSyscallObject[0]; if (type === 'callNow') { throw Error(`nodeWorker cannot block, cannot use syscall.callNow`); } + // This might throw an Error if the syscall was faulty, in which case + // the vat will be terminated soon. It returns a vatSyscallResults, + // which we discard because there is currently nobody to send it to. doSyscall(vatSyscallObject); } @@ -96,7 +104,8 @@ export function makeNodeSubprocessFactory(tools) { if (waiting) { const resolve = waiting; waiting = null; - resolve(); + const deliveryResult = args; + resolve(deliveryResult); } } else { parentLog(`unrecognized uplink message ${type}`); diff --git a/packages/xs-vat-worker/src/vatWorker.js b/packages/xs-vat-worker/src/vatWorker.js index cfdfc58a441..7933115f8c2 100644 --- a/packages/xs-vat-worker/src/vatWorker.js +++ b/packages/xs-vat-worker/src/vatWorker.js @@ -54,6 +54,16 @@ function makeWorker(io, setImmediate) { setImmediate, ); workerLog(`doProcess done`); + const vatDeliveryResults = harden(['ok']); + return vatDeliveryResults; + } + + function doMessage(targetSlot, msg) { + const errmsg = `vat[${targetSlot}].${msg.method} dispatch failed`; + return doProcess( + ['deliver', targetSlot, msg.method, msg.args, msg.result], + errmsg, + ); } function doNotify(vpid, vp) { @@ -82,7 +92,6 @@ function makeWorker(io, setImmediate) { // toParent.write('child ack'); // }); - let syscallLog; const handle = harden(async ([type, ...margs]) => { workerLog(`received`, type); if (type === 'start') { @@ -144,17 +153,12 @@ function makeWorker(io, setImmediate) { } const [dtype, ...dargs] = margs; if (dtype === 'message') { - const [targetSlot, msg] = dargs; - const errmsg = `vat[${targetSlot}].${msg.method} dispatch failed`; - await doProcess( - ['deliver', targetSlot, msg.method, msg.args, msg.result], - errmsg, - ).then(() => { - sendUplink(['deliverDone']); - }); + await doMessage(...dargs).then(res => + sendUplink(['deliverDone', ...res]), + ); } else if (dtype === 'notify') { - await doNotify(...dargs).then(() => - sendUplink(['deliverDone', syscallLog]), + await doNotify(...dargs).then(res => + sendUplink(['deliverDone', ...res]), ); } else { throw Error(`bad delivery type ${dtype}`); From 1edd62015e955d99fef8f75d32d2a5f1032aca38 Mon Sep 17 00:00:00 2001 From: Brian Warner Date: Wed, 16 Sep 2020 12:49:30 -0700 Subject: [PATCH 3/5] fix: xs-vat-worker: add makeMarshal to vatPowers to match other workers refs #1776 --- packages/xs-vat-worker/src/vatWorker.js | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/packages/xs-vat-worker/src/vatWorker.js b/packages/xs-vat-worker/src/vatWorker.js index 7933115f8c2..1c54042593a 100644 --- a/packages/xs-vat-worker/src/vatWorker.js +++ b/packages/xs-vat-worker/src/vatWorker.js @@ -1,6 +1,6 @@ /* global HandledPromise */ import { importBundle } from '@agoric/import-bundle'; -import { Remotable, getInterfaceOf } from '@agoric/marshal'; +import { Remotable, getInterfaceOf, makeMarshal } from '@agoric/marshal'; // TODO? import anylogger from 'anylogger'; import { makeLiveSlots } from '@agoric/swingset-vat/src/kernel/liveSlots'; @@ -133,7 +133,11 @@ 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, + makeMarshal, + }; dispatch = makeLiveSlots( syscall, state, From 29bc81a46d057532f51c37bed081d850cf7f31db Mon Sep 17 00:00:00 2001 From: Brian Warner Date: Wed, 16 Sep 2020 17:22:02 -0700 Subject: [PATCH 4/5] fix: pass testLog to all vatWorkers 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) --- packages/SwingSet/src/kernel/vatManager/factory.js | 3 +++ packages/SwingSet/src/kernel/vatManager/nodeWorker.js | 4 +++- .../src/kernel/vatManager/nodeWorkerSupervisor.js | 11 ++++++++++- .../src/kernel/vatManager/subprocessSupervisor.js | 11 ++++++++++- .../src/kernel/vatManager/worker-subprocess-node.js | 4 +++- packages/xs-vat-worker/src/vatWorker.js | 4 ++++ 6 files changed, 33 insertions(+), 4 deletions(-) diff --git a/packages/SwingSet/src/kernel/vatManager/factory.js b/packages/SwingSet/src/kernel/vatManager/factory.js index 8e720e2216f..8e6bbdf7714 100644 --- a/packages/SwingSet/src/kernel/vatManager/factory.js +++ b/packages/SwingSet/src/kernel/vatManager/factory.js @@ -27,16 +27,19 @@ export function makeVatManagerFactory({ const nodeWorkerFactory = makeNodeWorkerVatManagerFactory({ makeNodeWorker, kernelKeeper, + testLog: allVatPowers.testLog, }); const nodeSubprocessFactory = makeNodeSubprocessFactory({ startSubprocessWorker: startSubprocessWorkerNode, kernelKeeper, + testLog: allVatPowers.testLog, }); const xsWorkerFactory = makeNodeSubprocessFactory({ startSubprocessWorker: startSubprocessWorkerXS, kernelKeeper, + testLog: allVatPowers.testLog, }); function validateManagerOptions(managerOptions) { diff --git a/packages/SwingSet/src/kernel/vatManager/nodeWorker.js b/packages/SwingSet/src/kernel/vatManager/nodeWorker.js index 2f631d1dd5c..c2cc59e1d39 100644 --- a/packages/SwingSet/src/kernel/vatManager/nodeWorker.js +++ b/packages/SwingSet/src/kernel/vatManager/nodeWorker.js @@ -23,7 +23,7 @@ function parentLog(first, ...args) { } export function makeNodeWorkerVatManagerFactory(tools) { - const { makeNodeWorker, kernelKeeper } = tools; + const { makeNodeWorker, kernelKeeper, testLog } = tools; function createFromBundle(vatID, bundle, managerOptions) { const { vatParameters } = managerOptions; @@ -97,6 +97,8 @@ export function makeNodeWorkerVatManagerFactory(tools) { parentLog(`syscall`, args); const vatSyscallObject = args; handleSyscall(vatSyscallObject); + } else if (type === 'testLog') { + testLog(...args); } else if (type === 'deliverDone') { parentLog(`deliverDone`); if (waiting) { diff --git a/packages/SwingSet/src/kernel/vatManager/nodeWorkerSupervisor.js b/packages/SwingSet/src/kernel/vatManager/nodeWorkerSupervisor.js index b1e9955ca1b..12fbdfac3a9 100644 --- a/packages/SwingSet/src/kernel/vatManager/nodeWorkerSupervisor.js +++ b/packages/SwingSet/src/kernel/vatManager/nodeWorkerSupervisor.js @@ -105,13 +105,22 @@ parentPort.on('message', ([type, ...margs]) => { reject: (...args) => doSyscall(['reject', ...args]), }); + function testLog(...args) { + sendUplink(['testLog', ...args]); + } + const state = null; const vatID = 'demo-vatID'; // todo: maybe add transformTildot, makeGetMeter/transformMetering to // 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, makeMarshal }; + const vatPowers = { + Remotable, + getInterfaceOf, + makeMarshal, + testLog, + }; dispatch = makeLiveSlots( syscall, state, diff --git a/packages/SwingSet/src/kernel/vatManager/subprocessSupervisor.js b/packages/SwingSet/src/kernel/vatManager/subprocessSupervisor.js index 5a8c71abc85..5ed4a2dd7a3 100644 --- a/packages/SwingSet/src/kernel/vatManager/subprocessSupervisor.js +++ b/packages/SwingSet/src/kernel/vatManager/subprocessSupervisor.js @@ -120,13 +120,22 @@ fromParent.on('data', data => { reject: (...args) => doSyscall(['reject', ...args]), }); + function testLog(...args) { + sendUplink(['testLog', ...args]); + } + const state = null; const vatID = 'demo-vatID'; // todo: maybe add transformTildot, makeGetMeter/transformMetering to // 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, makeMarshal }; + const vatPowers = { + Remotable, + getInterfaceOf, + makeMarshal, + testLog, + }; dispatch = makeLiveSlots( syscall, state, diff --git a/packages/SwingSet/src/kernel/vatManager/worker-subprocess-node.js b/packages/SwingSet/src/kernel/vatManager/worker-subprocess-node.js index 90c4ebcf68d..d5d1bfc466a 100644 --- a/packages/SwingSet/src/kernel/vatManager/worker-subprocess-node.js +++ b/packages/SwingSet/src/kernel/vatManager/worker-subprocess-node.js @@ -24,7 +24,7 @@ function parentLog(first, ...args) { } export function makeNodeSubprocessFactory(tools) { - const { startSubprocessWorker, kernelKeeper } = tools; + const { startSubprocessWorker, kernelKeeper, testLog } = tools; function createFromBundle(vatID, bundle, managerOptions) { const { vatParameters } = managerOptions; @@ -99,6 +99,8 @@ export function makeNodeSubprocessFactory(tools) { parentLog(`syscall`, args); const vatSyscallObject = args; handleSyscall(vatSyscallObject); + } else if (type === 'testLog') { + testLog(...args); } else if (type === 'deliverDone') { parentLog(`deliverDone`); if (waiting) { diff --git a/packages/xs-vat-worker/src/vatWorker.js b/packages/xs-vat-worker/src/vatWorker.js index 1c54042593a..6aa128dd0bc 100644 --- a/packages/xs-vat-worker/src/vatWorker.js +++ b/packages/xs-vat-worker/src/vatWorker.js @@ -127,6 +127,9 @@ function makeWorker(io, setImmediate) { reject: (...args) => doSyscall(['reject', ...args]), }); + function testLog(...args) { + sendUplink(['testLog', ...args]); + } const state = null; const vatID = 'demo-vatID'; // todo: maybe add transformTildot, makeGetMeter/transformMetering to @@ -137,6 +140,7 @@ function makeWorker(io, setImmediate) { Remotable, getInterfaceOf, makeMarshal, + testLog, }; dispatch = makeLiveSlots( syscall, From 49605c62788d7bc6b85549fb7b9d4fe2ea172447 Mon Sep 17 00:00:00 2001 From: Brian Warner Date: Wed, 16 Sep 2020 18:12:16 -0700 Subject: [PATCH 5/5] test: exercise testLog in test-worker.js --- packages/SwingSet/test/workers/test-worker.js | 3 +++ packages/SwingSet/test/workers/vat-target.js | 5 ++++- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/packages/SwingSet/test/workers/test-worker.js b/packages/SwingSet/test/workers/test-worker.js index 854cc677aeb..ea709899487 100644 --- a/packages/SwingSet/test/workers/test-worker.js +++ b/packages/SwingSet/test/workers/test-worker.js @@ -15,6 +15,7 @@ maybeTestXS('xs vat manager', async t => { await c.run(); t.is(c.bootstrapResult.status(), 'fulfilled'); + t.deepEqual(c.dump().log, ['testLog works']); await c.shutdown(); }); @@ -28,6 +29,7 @@ test.skip('nodeWorker vat manager', async t => { await c.run(); t.is(c.bootstrapResult.status(), 'fulfilled'); + t.deepEqual(c.dump().log, ['testLog works']); await c.shutdown(); }); @@ -40,6 +42,7 @@ test('node-subprocess vat manager', async t => { await c.run(); t.is(c.bootstrapResult.status(), 'fulfilled'); + t.deepEqual(c.dump().log, ['testLog works']); await c.shutdown(); }); diff --git a/packages/SwingSet/test/workers/vat-target.js b/packages/SwingSet/test/workers/vat-target.js index 1af1ba458fb..dc28dc030d5 100644 --- a/packages/SwingSet/test/workers/vat-target.js +++ b/packages/SwingSet/test/workers/vat-target.js @@ -12,8 +12,11 @@ function ignore(p) { // inbound events ('dispatch'), which will provoke a set of outbound events // ('syscall'), that cover the full range of the dispatch/syscall interface -export function buildRootObject() { +export function buildRootObject(vatPowers) { console.log(`vat does buildRootObject`); // make sure console works + // note: XS doesn't appear to print console.log unless an exception happens + vatPowers.testLog('testLog works'); + const precB = makePromiseKit(); const precC = makePromiseKit(); let callbackObj;