From 2a242bd8ce3f3c90e792e5f60c4d2dd1d75f455a Mon Sep 17 00:00:00 2001 From: Brian Warner Date: Fri, 12 May 2023 01:07:24 -0700 Subject: [PATCH] fix(vats): fix promise space reset() misbehavior Previously, the reset() feature misbehaved in a common use pattern, where both the `reset` and `resolve` facets were extracted at the same time. The state was nominally held in `nameToState.get(name)`, but was sometimes used by closed-over `state` and `pk` variables. Which one you got depended upon when the Proxy `get` trap was called. If `resolve` was fetched early and retained across a `reset()` call, then `resolve()` would resolve the *old* promise. Later, when `consume` was used, the space would create a new Promise to satisfy the request, which would never be resolved. This changes the implementation to strictly keep/use the state in `nameToState`, and allow all resolve/reject/reset methods to work the same way no matter when they were retrieved (they close over `name` but not any state). closes #7709 --- packages/vats/src/core/promise-space.js | 107 +++++++++++------------ packages/vats/test/test-promise-space.js | 76 +++++++++++++++- 2 files changed, 125 insertions(+), 58 deletions(-) diff --git a/packages/vats/src/core/promise-space.js b/packages/vats/src/core/promise-space.js index 3a4044b9445..9f2f537b48d 100644 --- a/packages/vats/src/core/promise-space.js +++ b/packages/vats/src/core/promise-space.js @@ -1,4 +1,5 @@ // @ts-check +import { Fail } from '@agoric/assert'; import { assertKey } from '@agoric/store'; import { canBeDurable } from '@agoric/vat-data'; import { isPromise, makePromiseKit } from '@endo/promise-kit'; @@ -77,7 +78,7 @@ export const makeStoreHooks = (store, log = noop) => { * Make { produce, consume } where for each name, `consume[name]` is a promise * and `produce[name].resolve` resolves it. * - * Note: repeated resolve()s are noops. + * Note: repeated resolve()s without an intervening reset() are noops. * * @template {Record} [T=Record] * @param {{ log?: typeof console.log } & ( @@ -95,75 +96,69 @@ export const makePromiseSpace = (optsOrLog = {}) => { const { onAddKey, onSettled, onResolve, onReset } = hooks; /** - * @typedef {PromiseRecord & { - * reset: (reason?: unknown) => void, - * isSettling: boolean, - * }} PromiseState + * @typedef {{ pk: PromiseRecord, isSettling: boolean }} PromiseState */ /** @type {Map} */ const nameToState = new Map(); /** @type {Set} */ const remaining = new Set(); - /** @param {string} name */ + /** @type {(name: string) => PromiseState} */ const provideState = name => { - /** @type {PromiseState} */ - let state; - const currentState = nameToState.get(name); - if (currentState) { - state = currentState; - } else { + if (!nameToState.has(name)) { onAddKey(name); + remaining.add(name); const pk = makePromiseKit(); - pk.promise .finally(() => { - remaining.delete(name); onSettled(name, remaining); }) .catch(() => {}); + nameToState.set(name, harden({ pk, isSettling: false })); + } + return nameToState.get(name) || Fail`provideState(${name})`; + }; - const settling = () => { - assert(state); - state = harden({ ...state, isSettling: true }); - nameToState.set(name, state); - }; - - const resolve = value => { - settling(); - onResolve(name, value); - pk.resolve(value); - }; - const reject = reason => { - settling(); - pk.reject(reason); - }; - - const reset = (reason = undefined) => { - onReset(name); - if (!state.isSettling) { - if (!reason) { - // Reuse the old promise; don't reject it. - return; - } - reject(reason); + // we must tolerate these producer methods being retrieved both + // before and after the consumer is retrieved, and also both before + // and after reset() is invoked, so they only close over 'name' and + // not over any state variables + + const makeProducer = name => { + const resolve = value => { + onResolve(name, value); + const old = provideState(name); + nameToState.set(name, harden({ ...old, isSettling: true })); + old.pk.resolve(value); + remaining.delete(name); + }; + const reject = reason => { + const old = provideState(name); + nameToState.set(name, harden({ ...old, isSettling: true })); + old.pk.reject(reason); + remaining.delete(name); + }; + const reset = (reason = undefined) => { + onReset(name); + const old = provideState(name); + if (!old.isSettling) { + // we haven't produced a value yet, and there might be + // consumers still watching old.pk.promise + if (!reason) { + // so just let them wait for the new value: resetting an + // unresolved item is a no-op + return; } - // Now publish a new promise. - nameToState.delete(name); - remaining.delete(name); - }; + // reject those watchers; new watchers will wait for the new + // value through the replacement promise + reject(reason); + } + // delete the state, so new callers will get a new promise kit + nameToState.delete(name); + remaining.delete(name); + }; - state = harden({ - isSettling: false, - resolve, - reject, - reset, - promise: pk.promise, - }); - nameToState.set(name, state); - remaining.add(name); - } - return state; + return harden({ resolve, reject, reset }); }; /** @type {PromiseSpaceOf['consume']} */ @@ -173,8 +168,7 @@ export const makePromiseSpace = (optsOrLog = {}) => { { get: (_target, name) => { assert.typeof(name, 'string'); - const kit = provideState(name); - return kit.promise; + return provideState(name).pk.promise; }, }, ); @@ -186,8 +180,7 @@ export const makePromiseSpace = (optsOrLog = {}) => { { get: (_target, name) => { assert.typeof(name, 'string'); - const { reject, resolve, reset } = provideState(name); - return harden({ reject, resolve, reset }); + return makeProducer(name); }, }, ); diff --git a/packages/vats/test/test-promise-space.js b/packages/vats/test/test-promise-space.js index f415dddc3a8..0bdb4539b43 100644 --- a/packages/vats/test/test-promise-space.js +++ b/packages/vats/test/test-promise-space.js @@ -38,7 +38,7 @@ test('makePromiseSpace', async t => { await checkAlice(reusedAlice, `Hi, I'm Alice 3!`); }); -test('makePromiseSpace backed by store', async t => { +test('makePromiseSpace copied into store', async t => { /** @type {MapStore} */ const store = makeScalarBigMapStore('stuff', { durable: true }); { @@ -78,3 +78,77 @@ test('makePromiseSpace backed by store', async t => { produce.strawberry.resolve('ignored already resolved'); } }); + +test('resolve after reset', async t => { + /** @type {MapStore} */ + const store = makeScalarBigMapStore('stuff', { durable: true }); + + const { consume, produce } = makePromiseSpace({ store }); + + // sample resolve/consume early, then use after reset(): #7709 + + // for foo, we produce first + { + // reset before resolving the first time + const { resolve, reset } = produce.foo; + const foo1 = consume.foo; + reset(); + resolve(1); + t.is(await foo1, 1); + const foo2 = consume.foo; + t.is(await foo2, 1); + } + + { + const { resolve, reset } = produce.foo; + const foo1 = consume.foo; + reset(); + resolve(2); + t.is(await foo1, 1); // captured before reset() + const foo2 = consume.foo; + t.is(await foo2, 2); + } + + { + const foo1 = consume.foo; + produce.foo.reset(); + const foo2 = consume.foo; + produce.foo.resolve(3); + const foo3 = consume.foo; + t.is(await foo1, 2); // captured before reset() + t.is(await foo2, 3); + t.is(await foo3, 3); + } + + // for 'bar', we consume first + { + const bar1 = consume.bar; + const { resolve, reset } = produce.bar; + reset(); + resolve(1); + t.is(await bar1, 1); + const bar2 = consume.bar; + t.is(await bar2, 1); + } + + { + const { resolve, reset } = produce.bar; + const bar1 = consume.bar; + reset(); + resolve(2); + t.is(await bar1, 1); // captured before reset() + const bar2 = consume.bar; + t.is(await bar2, 2); + } + + { + const bar1 = consume.bar; + produce.bar.reset(); + const bar2 = consume.bar; + produce.bar.resolve(3); + const bar3 = consume.bar; + t.is(await bar1, 2); // captured before reset() + t.is(await bar2, 3); + t.is(await bar3, 3); + } +});