From afdfa2122538e215d21993860d5af60343cbd74e Mon Sep 17 00:00:00 2001 From: Erik Marks Date: Wed, 21 Feb 2024 14:44:58 -0800 Subject: [PATCH] refactor(daemon): Synchronize host evaluate method Synchronizes the host's `evaluate()` method by delegating all incarnations to the daemon via `incarnateEval()`. The latter is responsible for incarnating its dependents as necessary, and for the generation of their formula numbers. To facilitate this, the synchronous methods `incarnateLookupSync()` and `incarnateWorkerSync()` have been added. These methods synchronously mutate the formula graph, and return promises that resolve when the formulas have been written to disk. The result is that the formula graph is mutated within a single turn of the event loop. To achieve this, the implementation introduces new constraints on the daemon and its dependents. #2089 introduced the notion of "incarnating" values. By the current definition, incarnating a value consists of the following steps: 1. Collecting dependencies (async) a. Generating requisite formula numbers (async) - We use the asynchronous signature of `crypto.randomBytes` to do this for performance reasons. b. Incarnating any dependent values (recursion!) 2. Updating the in-memory formula graph (sync) 3. Writing the resulting formula to disk (async) 4. Reifiying the resulting value (async) In order to make formula graph mutations mutually exclusive, we introduce a "formula graph mutex" under which step 1 must be performed. This mutex is currently only used by `incarnateEval()`, and must be expanded to its sibling methods in the future. `incarnateEval()` also introduces the notion of "incarnation hooks" to the codebase. Originators of incarnations that are exogenous to the daemon may themselves have asynchronous work perform. For example, `petName -> formulaIdentifier` mappings are the responsibility of the host and its pet store, and pet names must be associated with their respective formula identifiers the moment that those identifiers are observable to the consumer. To handle sych asynchronous side effects, the implementation introduces a notion of "hooks" to `incarnateEval()`, with the intention of spreading this to other incarnation methods as necessary. These hooks receive as an argument all formula identifiers created by the incarnation, and are executed under the formula graph mutex. This will surface IO errors to the consumer, and help us uphold the principle of "death before confusion". Also of note, `provideValueForNumberedFormula` has been modified such that the formula is written to disk _after_ the controller has been constructed. This is critical in order to synchronize formula graph mutations. Finally, it appears that the implementation incidentally fixed #2021. We may still wish to adopt the more robust solution proposed in that issue. --- packages/daemon/src/daemon.js | 134 +++++++++++++++++++++++------- packages/daemon/src/host.js | 73 +++++++++++----- packages/daemon/src/mail.js | 15 +--- packages/daemon/src/types.d.ts | 16 ++-- packages/daemon/test/test-endo.js | 31 ++++--- 5 files changed, 188 insertions(+), 81 deletions(-) diff --git a/packages/daemon/src/daemon.js b/packages/daemon/src/daemon.js index 08f7e888ad..29954debcb 100644 --- a/packages/daemon/src/daemon.js +++ b/packages/daemon/src/daemon.js @@ -13,6 +13,7 @@ import { makeHostMaker } from './host.js'; import { assertPetName } from './pet-name.js'; import { makeContextMaker } from './context.js'; import { parseFormulaIdentifier } from './formula-identifier.js'; +import { makeMutex } from './mutex.js'; const delay = async (ms, cancelled) => { // Do not attempt to set up a timer if already cancelled. @@ -66,6 +67,7 @@ const makeDaemonCore = async ( } = powers; const { randomHex512 } = cryptoPowers; const contentStore = persistencePowers.makeContentSha512Store(); + const formulaGraphMutex = makeMutex(); /** * The two functions "provideValueForNumberedFormula" and "provideValueForFormulaIdentifier" @@ -549,10 +551,13 @@ const makeDaemonCore = async ( // Memoize for lookup. console.log(`Making ${formulaIdentifier}`); - const { promise: partial, resolve } = - /** @type {import('@endo/promise-kit').PromiseKit>} */ ( - makePromiseKit() - ); + const { + promise: partial, + resolve: resolvePartial, + reject: rejectPartial, + } = /** @type {import('@endo/promise-kit').PromiseKit>} */ ( + makePromiseKit() + ); // Behold, recursion: // eslint-disable-next-line no-use-before-define @@ -570,16 +575,22 @@ const makeDaemonCore = async ( }); controllerForFormulaIdentifier.set(formulaIdentifier, controller); - await persistencePowers.writeFormula(formula, formulaType, formulaNumber); - resolve( - makeControllerForFormula( - formulaIdentifier, - formulaNumber, - formula, - context, - ), + // We _must not_ await before the controller value is constructed. + const controllerValue = makeControllerForFormula( + formulaIdentifier, + formulaNumber, + formula, + context, ); + try { + await persistencePowers.writeFormula(formula, formulaType, formulaNumber); + } catch (error) { + rejectPartial(error); + throw error; + } + + resolvePartial(controllerValue); return harden({ formulaIdentifier, value: controller.external, @@ -716,6 +727,23 @@ const makeDaemonCore = async ( ); }; + /** + * Incarnates a `worker` formula and synchronously adds it to the formula graph. + * The returned promise is resolved after the formula is persisted. + * @param {string} formulaNumber - The worker formula number. + * @returns {Promise<{ formulaIdentifier: string, value: import('./types').EndoWorker }>} + */ + const incarnateWorkerSync = formulaNumber => { + /** @type {import('./types.js').WorkerFormula} */ + const formula = { + type: 'worker', + }; + + return /** @type {Promise<{ formulaIdentifier: string, value: import('./types').EndoWorker }>} */ ( + provideValueForNumberedFormula(formula.type, formulaNumber, formula) + ); + }; + /** * @param {string} endoFormulaIdentifier * @param {string} leastAuthorityFormulaIdentifier @@ -775,19 +803,60 @@ const makeDaemonCore = async ( }; /** - * @param {string} workerFormulaIdentifier + * @param {string} hostFormulaIdentifier * @param {string} source * @param {string[]} codeNames - * @param {string[]} endowmentFormulaIdentifiers + * @param {(string | string[])[]} endowmentFormulaPointers + * @param {import('./types.js').EvalFormulaHook[]} hooks + * @param {string} [specifiedWorkerFormulaIdentifier] * @returns {Promise<{ formulaIdentifier: string, value: unknown }>} */ const incarnateEval = async ( - workerFormulaIdentifier, + hostFormulaIdentifier, source, codeNames, - endowmentFormulaIdentifiers, + endowmentFormulaPointers, + hooks, + specifiedWorkerFormulaIdentifier, ) => { - const formulaNumber = await randomHex512(); + const { + workerFormulaIdentifier, + endowmentFormulaIdentifiers, + evalFormulaNumber, + } = await formulaGraphMutex.enqueue(async () => { + const ownFormulaNumber = await randomHex512(); + const workerFormulaNumber = await (specifiedWorkerFormulaIdentifier ?? + randomHex512()); + + const identifiers = harden({ + workerFormulaIdentifier: ( + await incarnateWorkerSync(workerFormulaNumber) + ).formulaIdentifier, + endowmentFormulaIdentifiers: await Promise.all( + endowmentFormulaPointers.map(async formulaIdOrPath => { + if (typeof formulaIdOrPath === 'string') { + return formulaIdOrPath; + } + return ( + /* eslint-disable no-use-before-define */ + ( + await incarnateLookupSync( + await randomHex512(), + hostFormulaIdentifier, + formulaIdOrPath, + ) + ).formulaIdentifier + /* eslint-enable no-use-before-define */ + ); + }), + ), + evalFormulaNumber: ownFormulaNumber, + }); + + await Promise.all(hooks.map(hook => hook(identifiers))); + return identifiers; + }); + /** @type {import('./types.js').EvalFormula} */ const formula = { type: 'eval', @@ -797,26 +866,33 @@ const makeDaemonCore = async ( values: endowmentFormulaIdentifiers, }; return /** @type {Promise<{ formulaIdentifier: string, value: unknown }>} */ ( - provideValueForNumberedFormula(formula.type, formulaNumber, formula) + provideValueForNumberedFormula(formula.type, evalFormulaNumber, formula) ); }; /** - * @param {string} hubFormulaIdentifier - * A "naming hub" is an objected with a variadic lookup method. It includes - * objects such as guests and hosts. - * @param {string[]} petNamePath - * @returns {Promise<{ formulaIdentifier: string, value: unknown }>} + * Incarnates a `lookup` formula and synchronously adds it to the formula graph. + * The returned promise is resolved after the formula is persisted. + * @param {string} formulaNumber - The lookup formula's number. + * @param {string} hubFormulaIdentifier - The formula identifier of the naming + * hub to call `lookup` on. A "naming hub" is an objected with a variadic + * lookup method. It includes objects such as guests and hosts. + * @param {string[]} petNamePath - The pet name path to look up. + * @returns {Promise<{ formulaIdentifier: string, value: import('./types').EndoWorker }>} */ - const incarnateLookup = async (hubFormulaIdentifier, petNamePath) => { - const formulaNumber = await randomHex512(); + const incarnateLookupSync = ( + formulaNumber, + hubFormulaIdentifier, + petNamePath, + ) => { /** @type {import('./types.js').LookupFormula} */ const formula = { type: 'lookup', hub: hubFormulaIdentifier, path: petNamePath, }; - return /** @type {Promise<{ formulaIdentifier: string, value: unknown }>} */ ( + + return /** @type {Promise<{ formulaIdentifier: string, value: import('./types.js').EndoWorker }>} */ ( provideValueForNumberedFormula(formula.type, formulaNumber, formula) ); }; @@ -938,11 +1014,11 @@ const makeDaemonCore = async ( }; /** - * @param {string} [specifiedFormulaNumber] + * @param {string} [specifiedFormulaNumber] - The formula number of the endo bootstrap. * @returns {Promise<{ formulaIdentifier: string, value: import('./types').FarEndoBootstrap }>} */ const incarnateEndoBootstrap = async specifiedFormulaNumber => { - const formulaNumber = specifiedFormulaNumber || (await randomHex512()); + const formulaNumber = await (specifiedFormulaNumber ?? randomHex512()); const endoFormulaIdentifier = `endo:${formulaNumber}`; const { formulaIdentifier: defaultHostWorkerFormulaIdentifier } = @@ -985,7 +1061,6 @@ const makeDaemonCore = async ( }); const makeMailbox = makeMailboxMaker({ - incarnateLookup, getFormulaIdentifierForRef, provideValueForFormulaIdentifier, provideControllerForFormulaIdentifier, @@ -1143,7 +1218,6 @@ const makeDaemonCore = async ( incarnateHost, incarnateGuest, incarnateEval, - incarnateLookup, incarnateUnconfined, incarnateReadableBlob, incarnateBundler, diff --git a/packages/daemon/src/host.js b/packages/daemon/src/host.js index cc9fbec9f8..1eec06814f 100644 --- a/packages/daemon/src/host.js +++ b/packages/daemon/src/host.js @@ -51,7 +51,6 @@ export const makeHostMaker = ({ reverseLookup, identifyLocal, listMessages, - provideLookupFormula, followMessages, resolve, reject, @@ -211,6 +210,7 @@ export const makeHostMaker = ({ await incarnateWorker(); return workerFormulaIdentifier; } + assertPetName(workerName); let workerFormulaIdentifier = identifyLocal(workerName); if (workerFormulaIdentifier === undefined) { @@ -222,6 +222,29 @@ export const makeHostMaker = ({ return workerFormulaIdentifier; }; + /** + * @param {string | 'MAIN' | 'NEW'} workerName + * @param {(hook: import('./types.js').EvalFormulaHook) => void} addHook + * @returns {string | undefined} + */ + const provideWorkerFormulaIdentifierSync = (workerName, addHook) => { + if (workerName === 'MAIN') { + return mainWorkerFormulaIdentifier; + } else if (workerName === 'NEW') { + return undefined; + } + + assertPetName(workerName); + const workerFormulaIdentifier = identifyLocal(workerName); + if (workerFormulaIdentifier === undefined) { + addHook(identifiers => + petStore.write(workerName, identifiers.workerFormulaIdentifier), + ); + return undefined; + } + return workerFormulaIdentifier; + }; + /** * @param {string | 'NONE' | 'SELF' | 'ENDO'} partyName * @returns {Promise} @@ -255,10 +278,6 @@ export const makeHostMaker = ({ petNamePaths, resultName, ) => { - const workerFormulaIdentifier = await provideWorkerFormulaIdentifier( - workerName, - ); - if (resultName !== undefined) { assertPetName(resultName); } @@ -266,8 +285,21 @@ export const makeHostMaker = ({ throw new Error('Evaluator requires one pet name for each code name'); } - const endowmentFormulaIdentifiers = await Promise.all( - petNamePaths.map(async (petNameOrPath, index) => { + /** @type {import('./types.js').EvalFormulaHook[]} */ + const hooks = []; + /** @type {(hook: import('./types.js').EvalFormulaHook) => void} */ + const addHook = hook => { + hooks.push(hook); + }; + + const workerFormulaIdentifier = provideWorkerFormulaIdentifierSync( + workerName, + addHook, + ); + + /** @type {(string | string[])[]} */ + const endowmentFormulaPointers = petNamePaths.map( + (petNameOrPath, index) => { if (typeof codeNames[index] !== 'string') { throw new Error(`Invalid endowment name: ${q(codeNames[index])}`); } @@ -281,23 +313,26 @@ export const makeHostMaker = ({ return formulaIdentifier; } - const { formulaIdentifier: lookupFormulaIdentifier } = - await provideLookupFormula(petNamePath); - return lookupFormulaIdentifier; - }), + // TODO:lookup Check if a formula already exists for the path. May have to be + // done in the daemon itself. + return petNamePath; + }, ); - // Behold, recursion: - // eslint-disable-next-line no-use-before-define - const { formulaIdentifier, value } = await incarnateEval( - workerFormulaIdentifier, + if (resultName !== undefined) { + addHook(identifiers => + petStore.write(resultName, `eval:${identifiers.evalFormulaNumber}`), + ); + } + + const { value } = await incarnateEval( + hostFormulaIdentifier, source, codeNames, - endowmentFormulaIdentifiers, + endowmentFormulaPointers, + hooks, + workerFormulaIdentifier, ); - if (resultName !== undefined) { - await petStore.write(resultName, formulaIdentifier); - } return value; }; diff --git a/packages/daemon/src/mail.js b/packages/daemon/src/mail.js index e8a0974427..a98f8657cd 100644 --- a/packages/daemon/src/mail.js +++ b/packages/daemon/src/mail.js @@ -10,14 +10,12 @@ const { quote: q } = assert; /** * @param {object} args - * @param {(hubFormulaIdentifier: string, petNamePath: string[]) => Promise<{ formulaIdentifier: string, value: unknown }>} args.incarnateLookup * @param {import('./types.js').ProvideValueForFormulaIdentifier} args.provideValueForFormulaIdentifier * @param {import('./types.js').ProvideControllerForFormulaIdentifier} args.provideControllerForFormulaIdentifier * @param {import('./types.js').GetFormulaIdentifierForRef} args.getFormulaIdentifierForRef * @param {import('./types.js').ProvideControllerForFormulaIdentifierAndResolveHandle} args.provideControllerForFormulaIdentifierAndResolveHandle */ export const makeMailboxMaker = ({ - incarnateLookup, getFormulaIdentifierForRef, provideValueForFormulaIdentifier, provideControllerForFormulaIdentifier, @@ -84,10 +82,8 @@ export const makeMailboxMaker = ({ throw new TypeError(`Unknown pet name: ${q(petName)}`); } // Behold, recursion: - // eslint-disable-next-line no-use-before-define - const controller = await provideControllerForFormulaIdentifier( - formulaIdentifier, - ); + const controller = + provideControllerForFormulaIdentifier(formulaIdentifier); console.log('Cancelled:'); return controller.context.cancel(reason); }; @@ -122,12 +118,6 @@ export const makeMailboxMaker = ({ return reverseLookupFormulaIdentifier(formulaIdentifier); }; - /** @type {import('./types.js').Mail['provideLookupFormula']} */ - const provideLookupFormula = async petNamePath => { - // TODO:lookup Check if the lookup formula already exists in the store - return incarnateLookup(selfFormulaIdentifier, petNamePath); - }; - /** * @param {import('./types.js').InternalMessage} message * @returns {import('./types.js').Message | undefined} @@ -541,7 +531,6 @@ export const makeMailboxMaker = ({ reverseLookup, reverseLookupFormulaIdentifier, identifyLocal, - provideLookupFormula, followMessages, listMessages, request, diff --git a/packages/daemon/src/types.d.ts b/packages/daemon/src/types.d.ts index c9e3b1b2e0..c8a2db33a0 100644 --- a/packages/daemon/src/types.d.ts +++ b/packages/daemon/src/types.d.ts @@ -97,6 +97,14 @@ type EvalFormula = { // TODO formula slots }; +export type EvalFormulaHook = ( + identifiers: Readonly<{ + endowmentFormulaIdentifiers: string[]; + evalFormulaNumber: string; + workerFormulaIdentifier: string; + }>, +) => Promise; + type ReadableBlobFormula = { type: 'readable-blob'; content: string; @@ -315,14 +323,6 @@ export interface Mail { listAll(): Array; reverseLookupFormulaIdentifier(formulaIdentifier: string): Array; cancel(petName: string, reason: unknown): Promise; - /** - * Takes a sequence of pet names and returns a formula identifier and value - * for the corresponding lookup formula. - * - * @param petNamePath A sequence of pet names. - * @returns The formula identifier and value of the lookup formula. - */ - provideLookupFormula(petNamePath: string[]): Promise; // Mail operations: listMessages(): Promise>; followMessages(): Promise>>; diff --git a/packages/daemon/test/test-endo.js b/packages/daemon/test/test-endo.js index 16a63f0108..82ddf6d525 100644 --- a/packages/daemon/test/test-endo.js +++ b/packages/daemon/test/test-endo.js @@ -880,8 +880,8 @@ test('name and reuse inspector', async t => { await stop(locator); }); -// TODO: This test verifies existing behavior when pet-naming workers. -// This behavior is undesirable. See: https://github.com/endojs/endo/issues/2021 +// This tests behavior that previously failed due to a bug. +// See: https://github.com/endojs/endo/issues/2021 test('eval-mediated worker name', async t => { const { promise: cancelled, reject: cancel } = makePromiseKit(); t.teardown(() => cancel(Error('teardown'))); @@ -903,6 +903,16 @@ test('eval-mediated worker name', async t => { const counterPath = path.join(dirname, 'test', 'counter.js'); await E(host).makeUnconfined('worker', counterPath, 'NONE', 'counter'); + t.is( + await E(host).evaluate( + 'worker', + 'E(counter).incr()', + ['counter'], + ['counter'], + ), + 1, + ); + // We create a petname for the worker of `counter`. // Note that while `worker === counter-worker`, it doesn't matter here. const counterWorker = await E(host).evaluate( @@ -914,19 +924,18 @@ test('eval-mediated worker name', async t => { ); t.regex(String(counterWorker), /Alleged: EndoWorker/u); - try { + // We should be able to use the worker. + t.is( await E(host).evaluate( - 'counter-worker', // Our worker pet name + 'counter-worker', 'E(counter).incr()', ['counter'], ['counter'], - ); - t.fail('should have thrown'); - } catch (error) { - // This is the error that we don't want - t.regex(error.message, /typeof target is "undefined"/u); - await stop(locator); - } + ), + 2, + ); + + await stop(locator); }); test('lookup with single petname', async t => {