From 8737a86f0c6e2bacfcb45d8a7d5642db8a141668 Mon Sep 17 00:00:00 2001 From: "Mark S. Miller" Date: Tue, 14 May 2024 14:51:31 -0700 Subject: [PATCH] fixup! reviewer discussion --- packages/async-flow/src/async-flow.js | 56 ++++++++++++++++++---- packages/async-flow/src/log-store.js | 4 ++ packages/async-flow/src/replay-membrane.js | 38 ++++++++------- 3 files changed, 71 insertions(+), 27 deletions(-) diff --git a/packages/async-flow/src/async-flow.js b/packages/async-flow/src/async-flow.js index f4f6504e994..433f2bdb26d 100644 --- a/packages/async-flow/src/async-flow.js +++ b/packages/async-flow/src/async-flow.js @@ -1,4 +1,4 @@ -import { annotateError, Fail, makeError, X } from '@endo/errors'; +import { annotateError, Fail, makeError, q, X } from '@endo/errors'; import { E } from '@endo/eventual-send'; import { M } from '@endo/patterns'; import { makeScalarWeakMapStore } from '@agoric/store'; @@ -257,13 +257,50 @@ export const prepareAsyncFlowTools = (outerZone, outerOptions = {}) => { const { flow } = facets; const flowState = flow.getFlowState(); - if (flowState === 'Done' || flowState === 'Failed') { - return; - } - if (hasMembrane(flow)) { - getMembrane(flow).wake(); - } else { - flow.restart(); + switch (flowState) { + case 'Done': + case 'Failed': { + return; + } + case 'Running': + case 'Replaying': { + // Safe to call membrane wake for a replaying or running flow + // because it is idempotent. membrane.wake already has reentrancy + // protection. Aside from harmless reentrancy, calling + // membrane.wake won't cause it to do anything that it would + // not have done on its own. + // + // An interesting edge case is that when the guest proceeds + // from a top-level doReturn or doThrow, while we're still in + // the guest turn, if somehow flow.wake were to be called then, + // and if the next thing in the replay log was a `doCall` + // (a future feature), then the `doCall` would call the guest + // while it was still in the middle of a "past" turn. However, + // this cannot happen because `flow` is host-side. For it to + // be called while the guest is active, the membrane's + // `callStack` would not be empty. membrane.wake checks and + // already throws an error in that case. + // + // More important, during a replay, no guest action can actually + // call host functions at all. Rather, the host is fully + // emulated from the log. So this case cannot arise. + // + // This analysis *assumes* that the guest function has no access + // to the flow outside the membrane, i.e., the "closed guest" + // assumption. + getMembrane(flow).wake(); + return; + } + case 'Sleeping': { + flow.restart(); + return; + } + default: { + // Should be a at-ts-expect-error that this case is unreachable + // which TS clearly knows anyway because it thinks the following + // `flowState` variable in this context has type `never`. + throw Fail`unexpected flowState ${q(flowState)}`; + } } }, getOutcome() { @@ -289,6 +326,7 @@ export const prepareAsyncFlowTools = (outerZone, outerOptions = {}) => { const { state, facets } = this; const { bijection, log } = state; const { flow } = facets; + !state.isDone || Fail`Cannot reset a done flow`; if (failures.has(flow)) { failures.delete(flow); @@ -299,8 +337,6 @@ export const prepareAsyncFlowTools = (outerZone, outerOptions = {}) => { } log.reset(); bijection.reset(); - - state.isDone = false; }, complete() { const { state, facets } = this; diff --git a/packages/async-flow/src/log-store.js b/packages/async-flow/src/log-store.js index 11a4c21633f..d0c7bbfa726 100644 --- a/packages/async-flow/src/log-store.js +++ b/packages/async-flow/src/log-store.js @@ -63,6 +63,10 @@ export const prepareLogStore = zone => { reset() { const { self } = this; tmp.resetFor(self); + + // TODO: Should we resolve replayDoneKit here, in case we're + // transitioning to a Failed state, so that any pending watchers + // can exit? }, dispose() { const { state, self } = this; diff --git a/packages/async-flow/src/replay-membrane.js b/packages/async-flow/src/replay-membrane.js index b56134a1d64..556a1a2bdb8 100644 --- a/packages/async-flow/src/replay-membrane.js +++ b/packages/async-flow/src/replay-membrane.js @@ -42,8 +42,8 @@ export const makeReplayMembrane = ( const doFulfill = (hostVow, hostFulfillment) => { const guestPromise = hostToGuest(hostVow); const status = guestPromiseMap.get(guestPromise); - if (status === 'settled') { - return; + if (!status || status === 'settled') { + Fail`doFulfill should only be called on a registered unresolved promise`; } const guestFulfillment = hostToGuest(hostFulfillment); status.resolve(guestFulfillment); @@ -61,8 +61,8 @@ export const makeReplayMembrane = ( const doReject = (hostVow, hostReason) => { const guestPromise = hostToGuest(hostVow); const status = guestPromiseMap.get(guestPromise); - if (status === 'settled') { - return; + if (!status || status === 'settled') { + Fail`doReject should only be called on a registered unresolved promise`; } const guestReason = hostToGuest(hostReason); status.reject(guestReason); @@ -184,14 +184,18 @@ export const makeReplayMembrane = ( throw panic(fatalError); } - if (outcome.kind === 'return') { - return outcome.result; - } else { - outcome.kind === 'throw' || + switch (outcome.kind) { + case 'return': { + return outcome.result; + } + case 'throw': { + throw outcome.problem; + } + default: { // @ts-expect-error TS correctly knows this case would be outside // the type. But that's what we want to check. - Fail`unexpected outcome kind ${q(outcome.kind)}`; - throw outcome.problem; + throw Fail`unexpected outcome kind ${q(outcome.kind)}`; + } } }; @@ -213,7 +217,7 @@ export const makeReplayMembrane = ( }; if (optVerb) { defineProperties(guestMethod, { - name: { value: String(optVerb) }, + name: { value: String(hRem[optVerb].name || optVerb) }, length: { value: Number(hRem[optVerb].length || 0) }, }); } else { @@ -263,21 +267,21 @@ export const makeReplayMembrane = ( void when( hVow, async hostFulfillment => { - await log.promiseReplayDone(); - if (guestPromiseMap.get(promise) !== 'settled') { + await log.promiseReplayDone(); // should never reject + if (!stopped && guestPromiseMap.get(promise) !== 'settled') { /** @type {LogEntry} */ const entry = harden(['doFulfill', hVow, hostFulfillment]); log.pushEntry(entry); - interpretOne(topDispatch, entry); + interpretOne(topDispatch, entry); // does its own panic } }, async hostReason => { - await log.promiseReplayDone(); - if (guestPromiseMap.get(promise) !== 'settled') { + await log.promiseReplayDone(); // should never reject + if (!stopped && guestPromiseMap.get(promise) !== 'settled') { /** @type {LogEntry} */ const entry = harden(['doReject', hVow, hostReason]); log.pushEntry(entry); - interpretOne(topDispatch, entry); + interpretOne(topDispatch, entry); // does its own panic } }, );