From c191ee185d8f43b083fb2e3958859f20f0299dda Mon Sep 17 00:00:00 2001 From: Brian Warner Date: Fri, 12 May 2023 01:07:24 -0700 Subject: [PATCH 1/5] 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 | 108 +++++++++++------------ packages/vats/test/test-promise-space.js | 76 +++++++++++++++- 2 files changed, 126 insertions(+), 58 deletions(-) diff --git a/packages/vats/src/core/promise-space.js b/packages/vats/src/core/promise-space.js index 3a4044b9445..f6a88785e3f 100644 --- a/packages/vats/src/core/promise-space.js +++ b/packages/vats/src/core/promise-space.js @@ -77,7 +77,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,77 +95,73 @@ 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 { + let state = nameToState.get(name); + if (!state) { onAddKey(name); + remaining.add(name); const pk = makePromiseKit(); - pk.promise .finally(() => { - remaining.delete(name); onSettled(name, remaining); }) .catch(() => {}); - - 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); - } - // Now publish a new promise. - nameToState.delete(name); - remaining.delete(name); - }; - - state = harden({ - isSettling: false, - resolve, - reject, - reset, - promise: pk.promise, - }); + state = harden({ pk, isSettling: false }); nameToState.set(name, state); - remaining.add(name); } return state; }; + // 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 === undefined) { + // so just let them wait for the new value: resetting an + // unresolved item is a no-op + return; + } + // 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); + }; + + return harden({ resolve, reject, reset }); + }; + /** @type {PromiseSpaceOf['consume']} */ // @ts-expect-error cast const consume = new Proxy( @@ -173,8 +169,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 +181,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); + } +}); From ad64a44442d86900b4220bf8def00d04099d349f Mon Sep 17 00:00:00 2001 From: Brian Warner Date: Fri, 12 May 2023 11:34:03 -0700 Subject: [PATCH 2/5] chore: harden the promise-space Proxy targets per @erights: this prevents non-get operations from mutating the target and using it as a communications channel. --- packages/vats/src/core/promise-space.js | 26 ++++++++++--------------- 1 file changed, 10 insertions(+), 16 deletions(-) diff --git a/packages/vats/src/core/promise-space.js b/packages/vats/src/core/promise-space.js index f6a88785e3f..8d05e904c4b 100644 --- a/packages/vats/src/core/promise-space.js +++ b/packages/vats/src/core/promise-space.js @@ -164,27 +164,21 @@ export const makePromiseSpace = (optsOrLog = {}) => { /** @type {PromiseSpaceOf['consume']} */ // @ts-expect-error cast - const consume = new Proxy( - {}, - { - get: (_target, name) => { - assert.typeof(name, 'string'); - return provideState(name).pk.promise; - }, + const consume = new Proxy(harden({}), { + get: (_target, name) => { + assert.typeof(name, 'string'); + return provideState(name).pk.promise; }, - ); + }); /** @type {PromiseSpaceOf['produce']} */ // @ts-expect-error cast - const produce = new Proxy( - {}, - { - get: (_target, name) => { - assert.typeof(name, 'string'); - return makeProducer(name); - }, + const produce = new Proxy(harden({}), { + get: (_target, name) => { + assert.typeof(name, 'string'); + return makeProducer(name); }, - ); + }); return harden({ produce, consume }); }; From b4b7f8990bf286687bd98f5589931dd45e452e84 Mon Sep 17 00:00:00 2001 From: Brian Warner Date: Fri, 12 May 2023 12:10:22 -0700 Subject: [PATCH 3/5] chore: do remaining.delete in the right place --- packages/vats/src/core/promise-space.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/packages/vats/src/core/promise-space.js b/packages/vats/src/core/promise-space.js index 8d05e904c4b..9abbd92e0be 100644 --- a/packages/vats/src/core/promise-space.js +++ b/packages/vats/src/core/promise-space.js @@ -111,6 +111,7 @@ export const makePromiseSpace = (optsOrLog = {}) => { const pk = makePromiseKit(); pk.promise .finally(() => { + remaining.delete(name); onSettled(name, remaining); }) .catch(() => {}); @@ -131,13 +132,11 @@ export const makePromiseSpace = (optsOrLog = {}) => { 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); From 46c6c8766adffb52e5d11109e437c4e8cb12bc5e Mon Sep 17 00:00:00 2001 From: Brian Warner Date: Fri, 12 May 2023 12:16:12 -0700 Subject: [PATCH 4/5] chore: switch to E.when for promise-space onResolve hook This is more defensive against sneaky promises, at the cost of delaying storage by a turn for non-promises. --- packages/vats/src/core/promise-space.js | 9 +++------ packages/vats/test/test-promise-space.js | 1 + 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/packages/vats/src/core/promise-space.js b/packages/vats/src/core/promise-space.js index 9abbd92e0be..3eec095d7ea 100644 --- a/packages/vats/src/core/promise-space.js +++ b/packages/vats/src/core/promise-space.js @@ -1,7 +1,8 @@ // @ts-check +import { E } from '@endo/far'; import { assertKey } from '@agoric/store'; import { canBeDurable } from '@agoric/vat-data'; -import { isPromise, makePromiseKit } from '@endo/promise-kit'; +import { makePromiseKit } from '@endo/promise-kit'; /** * @typedef {{ @@ -59,11 +60,7 @@ export const makeStoreHooks = (store, log = noop) => { return harden({ ...logHooks, onResolve: (name, valueP) => { - if (isPromise(valueP)) { - void valueP.then(value => save(name, value)); - } else { - save(name, valueP); - } + void E.when(valueP, value => save(name, value)); }, onReset: name => { if (store.has(name)) { diff --git a/packages/vats/test/test-promise-space.js b/packages/vats/test/test-promise-space.js index 0bdb4539b43..38b7fda8fa8 100644 --- a/packages/vats/test/test-promise-space.js +++ b/packages/vats/test/test-promise-space.js @@ -64,6 +64,7 @@ test('makePromiseSpace copied into store', async t => { produce.chocolate.resolve(doesNotResolve); const nonPassable = harden(() => {}); produce.strawberry.resolve(nonPassable); + await null; const actual = Object.fromEntries(store.entries()); t.deepEqual( actual, From e758c3450ea59f6d13d1f6654dbc57a5462bd026 Mon Sep 17 00:00:00 2001 From: Brian Warner Date: Fri, 12 May 2023 13:13:19 -0700 Subject: [PATCH 5/5] chore: promise-space: ignore multiple resolutions Co-authored-by: Michael FIG --- packages/vats/src/core/promise-space.js | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/packages/vats/src/core/promise-space.js b/packages/vats/src/core/promise-space.js index 3eec095d7ea..309a3afb156 100644 --- a/packages/vats/src/core/promise-space.js +++ b/packages/vats/src/core/promise-space.js @@ -125,13 +125,21 @@ export const makePromiseSpace = (optsOrLog = {}) => { const makeProducer = name => { const resolve = value => { - onResolve(name, value); const old = provideState(name); + if (old.isSettling) { + // First attempt to settle always wins. + return; + } + onResolve(name, value); nameToState.set(name, harden({ ...old, isSettling: true })); old.pk.resolve(value); }; const reject = reason => { const old = provideState(name); + if (old.isSettling) { + // First attempt to settle always wins. + return; + } nameToState.set(name, harden({ ...old, isSettling: true })); old.pk.reject(reason); };