From 3b585a2ead2b2d3c2402a406470b33b78b170434 Mon Sep 17 00:00:00 2001 From: Erik Marks Date: Tue, 20 Feb 2024 12:09:23 -0800 Subject: [PATCH 1/4] refactor(daemon): Make makeControllerForFormula synchronous Makes `makeControllerForFormula` and its dependent functions synchronous. Also performs some unused parameter cleanup. --- packages/daemon/src/daemon.js | 24 ++++++++++-------------- packages/daemon/src/host.js | 1 - 2 files changed, 10 insertions(+), 15 deletions(-) diff --git a/packages/daemon/src/daemon.js b/packages/daemon/src/daemon.js index b929dac396..08f7e888ad 100644 --- a/packages/daemon/src/daemon.js +++ b/packages/daemon/src/daemon.js @@ -49,7 +49,6 @@ const makeInspector = (type, number, record) => * @param {import('./types.js').DaemonicPowers} powers * @param {Promise} webletPortP * @param {object} args - * @param {Promise} args.cancelled * @param {(error: Error) => void} args.cancel * @param {number} args.gracePeriodMs * @param {Promise} args.gracePeriodElapsed @@ -57,7 +56,7 @@ const makeInspector = (type, number, record) => const makeDaemonCore = async ( powers, webletPortP, - { cancelled, cancel, gracePeriodMs, gracePeriodElapsed }, + { cancel, gracePeriodMs, gracePeriodElapsed }, ) => { const { crypto: cryptoPowers, @@ -336,7 +335,7 @@ const makeDaemonCore = async ( * @param {import('./types.js').Formula} formula * @param {import('./types.js').Context} context */ - const makeControllerForFormula = async ( + const makeControllerForFormula = ( formulaIdentifier, formulaNumber, formula, @@ -622,15 +621,16 @@ const makeDaemonCore = async ( }; /** @type {import('./types.js').ProvideValueForFormulaIdentifier} */ - const provideValueForFormulaIdentifier = async formulaIdentifier => { + const provideValueForFormulaIdentifier = formulaIdentifier => { const controller = /** @type {import('./types.js').Controller<>} */ ( provideControllerForFormulaIdentifier(formulaIdentifier) ); - const value = await controller.external; - if (typeof value === 'object' && value !== null) { - formulaIdentifierForRef.set(value, formulaIdentifier); - } - return value; + return controller.external.then(value => { + if (typeof value === 'object' && value !== null) { + formulaIdentifierForRef.set(value, formulaIdentifier); + } + return value; + }); }; /** @type {import('./types.js').ProvideControllerForFormulaIdentifierAndResolveHandle} */ @@ -1010,7 +1010,6 @@ const makeDaemonCore = async ( incarnateWebBundle, incarnateHandle, storeReaderRef, - randomHex512, makeMailbox, }); @@ -1159,7 +1158,6 @@ const makeDaemonCore = async ( * @param {import('./types.js').DaemonicPowers} powers * @param {Promise} webletPortP * @param {object} args - * @param {Promise} args.cancelled * @param {(error: Error) => void} args.cancel * @param {number} args.gracePeriodMs * @param {Promise} args.gracePeriodElapsed @@ -1168,12 +1166,11 @@ const makeDaemonCore = async ( const provideEndoBootstrap = async ( powers, webletPortP, - { cancelled, cancel, gracePeriodMs, gracePeriodElapsed }, + { cancel, gracePeriodMs, gracePeriodElapsed }, ) => { const { persistence: persistencePowers } = powers; const daemonCore = await makeDaemonCore(powers, webletPortP, { - cancelled, cancel, gracePeriodMs, gracePeriodElapsed, @@ -1228,7 +1225,6 @@ export const makeDaemon = async (powers, daemonLabel, cancel, cancelled) => { powers, assignedWebletPortP, { - cancelled, cancel, gracePeriodMs, gracePeriodElapsed, diff --git a/packages/daemon/src/host.js b/packages/daemon/src/host.js index c0cf74bc42..cc9fbec9f8 100644 --- a/packages/daemon/src/host.js +++ b/packages/daemon/src/host.js @@ -17,7 +17,6 @@ export const makeHostMaker = ({ incarnateWebBundle, incarnateHandle, storeReaderRef, - randomHex512, makeMailbox, }) => { /** From 728c01602a19bcc4086e44982ebd43f643a95bc7 Mon Sep 17 00:00:00 2001 From: Erik Marks Date: Wed, 21 Feb 2024 14:44:58 -0800 Subject: [PATCH 2/4] 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..b08b925cdd 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 new name for 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 => { From 93d022a1f276e3e289eb26e718ce8e912998ae2c Mon Sep 17 00:00:00 2001 From: Erik Marks Date: Wed, 21 Feb 2024 20:27:38 -0800 Subject: [PATCH 3/4] fix(daemon): Correctly use worker formula number in incarnateEval `incarnateEval()` was accidentally passing worker formula identifiers instead of formula numbers when it received a specified worker formula identifier. This caused `eval` formulas to always fail on Windows due to a `:` in the path. Surprisingly, fixing this caused the "indirect termination" test to fail. This is likely just surfacing a deeper problem, see #2074. --- packages/daemon/src/daemon.js | 5 +++-- packages/daemon/test/test-endo.js | 3 ++- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/packages/daemon/src/daemon.js b/packages/daemon/src/daemon.js index 29954debcb..cfa88c553e 100644 --- a/packages/daemon/src/daemon.js +++ b/packages/daemon/src/daemon.js @@ -825,8 +825,9 @@ const makeDaemonCore = async ( evalFormulaNumber, } = await formulaGraphMutex.enqueue(async () => { const ownFormulaNumber = await randomHex512(); - const workerFormulaNumber = await (specifiedWorkerFormulaIdentifier ?? - randomHex512()); + const workerFormulaNumber = await (specifiedWorkerFormulaIdentifier + ? parseFormulaIdentifier(specifiedWorkerFormulaIdentifier).number + : randomHex512()); const identifiers = harden({ workerFormulaIdentifier: ( diff --git a/packages/daemon/test/test-endo.js b/packages/daemon/test/test-endo.js index b08b925cdd..be79a2305c 100644 --- a/packages/daemon/test/test-endo.js +++ b/packages/daemon/test/test-endo.js @@ -647,7 +647,8 @@ test('direct termination', async t => { t.pass(); }); -test('indirect termination', async t => { +// See: https://github.com/endojs/endo/issues/2074 +test.failing('indirect termination', async t => { const { promise: cancelled, reject: cancel } = makePromiseKit(); t.teardown(() => cancel(Error('teardown'))); From fcd9093a50a42f8f64e217d9365cb12067bf7480 Mon Sep 17 00:00:00 2001 From: Erik Marks <25517051+rekmarks@users.noreply.github.com> Date: Wed, 21 Feb 2024 20:37:46 -0800 Subject: [PATCH 4/4] refactor(daemon): Refine synchronized `incarnateEval` implementation - Rename various entities for clarity / correctness - Use terser implementation of async flow in `provideValueforNumberedFormula` - Add inline documentation Co-authored-by: Kris Kowal --- packages/daemon/src/daemon.js | 45 ++++++++++++++++--------------- packages/daemon/src/host.js | 6 ++--- packages/daemon/test/test-endo.js | 3 +-- 3 files changed, 26 insertions(+), 28 deletions(-) diff --git a/packages/daemon/src/daemon.js b/packages/daemon/src/daemon.js index cfa88c553e..3dc5053175 100644 --- a/packages/daemon/src/daemon.js +++ b/packages/daemon/src/daemon.js @@ -551,13 +551,10 @@ const makeDaemonCore = async ( // Memoize for lookup. console.log(`Making ${formulaIdentifier}`); - const { - promise: partial, - resolve: resolvePartial, - reject: rejectPartial, - } = /** @type {import('@endo/promise-kit').PromiseKit>} */ ( - makePromiseKit() - ); + const { promise: partial, resolve: resolvePartial } = + /** @type {import('@endo/promise-kit').PromiseKit>} */ ( + makePromiseKit() + ); // Behold, recursion: // eslint-disable-next-line no-use-before-define @@ -575,7 +572,7 @@ const makeDaemonCore = async ( }); controllerForFormulaIdentifier.set(formulaIdentifier, controller); - // We _must not_ await before the controller value is constructed. + // The controller _must_ be constructed in the synchronous prelude of this function. const controllerValue = makeControllerForFormula( formulaIdentifier, formulaNumber, @@ -583,14 +580,16 @@ const makeDaemonCore = async ( context, ); - try { - await persistencePowers.writeFormula(formula, formulaType, formulaNumber); - } catch (error) { - rejectPartial(error); - throw error; - } + // Ensure that failure to flush the formula to storage + // causes a rejection for both the controller and the incarnation value. + const written = persistencePowers.writeFormula( + formula, + formulaType, + formulaNumber, + ); + resolvePartial(written.then(() => /** @type {any} */ (controllerValue))); + await written; - resolvePartial(controllerValue); return harden({ formulaIdentifier, value: controller.external, @@ -637,6 +636,8 @@ const makeDaemonCore = async ( provideControllerForFormulaIdentifier(formulaIdentifier) ); return controller.external.then(value => { + // Release the value to the public only after ensuring + // we can reverse-lookup its nonce. if (typeof value === 'object' && value !== null) { formulaIdentifierForRef.set(value, formulaIdentifier); } @@ -733,7 +734,7 @@ const makeDaemonCore = async ( * @param {string} formulaNumber - The worker formula number. * @returns {Promise<{ formulaIdentifier: string, value: import('./types').EndoWorker }>} */ - const incarnateWorkerSync = formulaNumber => { + const incarnateNumberedWorker = formulaNumber => { /** @type {import('./types.js').WorkerFormula} */ const formula = { type: 'worker', @@ -806,7 +807,7 @@ const makeDaemonCore = async ( * @param {string} hostFormulaIdentifier * @param {string} source * @param {string[]} codeNames - * @param {(string | string[])[]} endowmentFormulaPointers + * @param {(string | string[])[]} endowmentFormulaIdsOrPaths * @param {import('./types.js').EvalFormulaHook[]} hooks * @param {string} [specifiedWorkerFormulaIdentifier] * @returns {Promise<{ formulaIdentifier: string, value: unknown }>} @@ -815,7 +816,7 @@ const makeDaemonCore = async ( hostFormulaIdentifier, source, codeNames, - endowmentFormulaPointers, + endowmentFormulaIdsOrPaths, hooks, specifiedWorkerFormulaIdentifier, ) => { @@ -831,17 +832,17 @@ const makeDaemonCore = async ( const identifiers = harden({ workerFormulaIdentifier: ( - await incarnateWorkerSync(workerFormulaNumber) + await incarnateNumberedWorker(workerFormulaNumber) ).formulaIdentifier, endowmentFormulaIdentifiers: await Promise.all( - endowmentFormulaPointers.map(async formulaIdOrPath => { + endowmentFormulaIdsOrPaths.map(async formulaIdOrPath => { if (typeof formulaIdOrPath === 'string') { return formulaIdOrPath; } return ( /* eslint-disable no-use-before-define */ ( - await incarnateLookupSync( + await incarnateNumberedLookup( await randomHex512(), hostFormulaIdentifier, formulaIdOrPath, @@ -881,7 +882,7 @@ const makeDaemonCore = async ( * @param {string[]} petNamePath - The pet name path to look up. * @returns {Promise<{ formulaIdentifier: string, value: import('./types').EndoWorker }>} */ - const incarnateLookupSync = ( + const incarnateNumberedLookup = ( formulaNumber, hubFormulaIdentifier, petNamePath, diff --git a/packages/daemon/src/host.js b/packages/daemon/src/host.js index 1eec06814f..29cb7c72ab 100644 --- a/packages/daemon/src/host.js +++ b/packages/daemon/src/host.js @@ -298,7 +298,7 @@ export const makeHostMaker = ({ ); /** @type {(string | string[])[]} */ - const endowmentFormulaPointers = petNamePaths.map( + const endowmentFormulaIdsOrPaths = petNamePaths.map( (petNameOrPath, index) => { if (typeof codeNames[index] !== 'string') { throw new Error(`Invalid endowment name: ${q(codeNames[index])}`); @@ -313,8 +313,6 @@ export const makeHostMaker = ({ return formulaIdentifier; } - // TODO:lookup Check if a formula already exists for the path. May have to be - // done in the daemon itself. return petNamePath; }, ); @@ -329,7 +327,7 @@ export const makeHostMaker = ({ hostFormulaIdentifier, source, codeNames, - endowmentFormulaPointers, + endowmentFormulaIdsOrPaths, hooks, workerFormulaIdentifier, ); diff --git a/packages/daemon/test/test-endo.js b/packages/daemon/test/test-endo.js index be79a2305c..c0abd773b4 100644 --- a/packages/daemon/test/test-endo.js +++ b/packages/daemon/test/test-endo.js @@ -881,8 +881,7 @@ test('name and reuse inspector', async t => { await stop(locator); }); -// This tests behavior that previously failed due to a bug. -// See: https://github.com/endojs/endo/issues/2021 +// Regression test for: 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')));