feat: vat-side promise ID retirement

packages/SwingSet/src/kernel/cleanup.js

@@ -1,67 +1,6 @@
 // import { kdebug } from './kdebug';
 import { parseKernelSlot } from './parseKernelSlots';
 
-// XXX temporary flags to control features during development
-const ENABLE_PROMISE_ANALYSIS = true; // flag to enable/disable check to see if delete clist entry is ok
-
-export function deleteCListEntryIfEasy(
-  vatID,
-  vatKeeper,
-  kernelKeeper,
-  kpid,
-  vpid,
-  kernelData,
-) {
-  if (ENABLE_PROMISE_ANALYSIS) {
-    const visited = new Set();
-    let sawPromise;
-
-    function scanKernelPromise(scanKPID, scanKernelData) {
-      visited.add(scanKPID);
-      // kdebug(`@@@ scan ${scanKPID} ${JSON.stringify(scanKernelData)}`);
-      if (scanKernelData) {
-        for (const slot of scanKernelData.slots) {
-          const { type } = parseKernelSlot(slot);
-          if (type === 'promise') {
-            sawPromise = slot;
-            if (visited.has(slot)) {
-              // kdebug(`@@@ ${slot} previously visited`);
-              return true;
-            } else {
-              const { data } = kernelKeeper.getKernelPromise(slot);
-              // const { data, state } = kernelKeeper.getKernelPromise(slot);
-              if (data) {
-                if (scanKernelPromise(slot, data)) {
-                  // kdebug(`@@@ scan ${slot} detects circularity`);
-                  return true;
-                }
-              } else {
-                // kdebug(`@@@ scan ${slot} state = ${state}`);
-              }
-            }
-          }
-        }
-      }
-      // kdebug(`@@@ scan ${scanKPID} detects no circularity`);
-      return false;
-    }
-
-    // kdebug(`@@ checking ${vatID} ${kpid} for circularity`);
-    if (scanKernelPromise(kpid, kernelData)) {
-      // kdebug(
-      //  `Unable to delete ${vatID} clist entry ${kpid}<=>${vpid} because it is indirectly self-referential`,
-      // );
-      return;
-    } else if (sawPromise) {
-      // kdebug(
-      //  `Unable to delete ${vatID} clist entry ${kpid}<=>${vpid} because there was a contained promise ${sawPromise}`,
-      // );
-      return;
-    }
-  }
-  vatKeeper.deleteCListEntry(kpid, vpid);
-}
-
 export function getKpidsToRetire(kernelKeeper, rootKPID, rootKernelData) {
   const seen = new Set();
   function scanKernelPromise(kpid, kernelData) {

packages/SwingSet/src/kernel/liveSlots.js

@@ -195,11 +195,15 @@ function build(syscall, forVatID, cacheSize, vatPowers, vatParameters) {
     return makeVatSlot('promise', true, promiseID);
   }
 
+  const knownResolutions = new WeakMap();
+
   function exportPromise(p) {
     const pid = allocatePromiseID();
     lsdebug(`Promise allocation ${forVatID}:${pid} in exportPromise`);
-    // eslint-disable-next-line no-use-before-define
-    p.then(thenResolve(pid), thenReject(pid));
+    if (!knownResolutions.has(p)) {
+      // eslint-disable-next-line no-use-before-define
+      p.then(thenResolve(p, pid), thenReject(p, pid));
+    }
     return pid;
   }
 
@@ -311,6 +315,49 @@ function build(syscall, forVatID, cacheSize, vatPowers, vatParameters) {
     return val;
   }
 
+  function resolutionCollector() {
+    const resolutions = [];
+    const doneResolutions = new Set();
+
+    function scanSlots(slots) {
+      for (const slot of slots) {
+        const { type } = parseVatSlot(slot);
+        if (type === 'promise') {
+          const p = slotToVal.get(slot);
+          assert(p, details`should have a value for ${slot} but didn't`);
+          const priorResolution = knownResolutions.get(p);
+          if (priorResolution && !doneResolutions.has(slot)) {
+            const [priorRejected, priorRes] = priorResolution;
+            // eslint-disable-next-line no-use-before-define
+            collect(slot, priorRejected, priorRes);
+          }
+        }
+      }
+    }
+
+    function collect(promiseID, rejected, value) {
+      doneResolutions.add(promiseID);
+      const valueSer = m.serialize(value);
+      resolutions.push([promiseID, rejected, valueSer]);
+      scanSlots(valueSer.slots);
+    }
+
+    function forPromise(promiseID, rejected, value) {
+      collect(promiseID, rejected, value);
+      return resolutions;
+    }
+
+    function forSlots(slots) {
+      scanSlots(slots);
+      return resolutions;
+    }
+
+    return {
+      forPromise,
+      forSlots,
+    };
+  }
+
   function queueMessage(targetSlot, prop, args, returnedP) {
     const serArgs = m.serialize(harden(args));
     const resultVPID = allocatePromiseID();
@@ -326,6 +373,10 @@ function build(syscall, forVatID, cacheSize, vatPowers, vatParameters) {
       )}) -> ${resultVPID}`,
     );
     syscall.send(targetSlot, prop, serArgs, resultVPID);
+    const resolutions = resolutionCollector().forSlots(serArgs.slots);
+    if (resolutions.length > 0) {
+      syscall.resolve(resolutions);
+    }
 
     // ideally we'd wait until .then is called on p before subscribing, but
     // the current Promise API doesn't give us a way to discover this, so we
@@ -410,16 +461,6 @@ function build(syscall, forVatID, cacheSize, vatPowers, vatParameters) {
 
     const args = m.unserialize(argsdata);
 
-    let notifySuccess = () => undefined;
-    let notifyFailure = () => undefined;
-    if (result) {
-      insistVatType('promise', result);
-      // eslint-disable-next-line no-use-before-define
-      notifySuccess = thenResolve(result);
-      // eslint-disable-next-line no-use-before-define
-      notifyFailure = thenReject(result);
-    }
-
     // If the method is missing, or is not a Function, or the method throws a
     // synchronous exception, we notify the caller (by rejecting the result
     // promise, if any). If the method returns an eventually-rejected
@@ -442,6 +483,15 @@ function build(syscall, forVatID, cacheSize, vatPowers, vatParameters) {
       // TODO: untested, but in principle sound.
       res = HandledPromise.get(t, method);
     }
+    let notifySuccess = () => undefined;
+    let notifyFailure = () => undefined;
+    if (result) {
+      insistVatType('promise', result);
+      // eslint-disable-next-line no-use-before-define
+      notifySuccess = thenResolve(res, result);
+      // eslint-disable-next-line no-use-before-define
+      notifyFailure = thenReject(res, result);
+    }
     res.then(notifySuccess, notifyFailure);
   }
 
@@ -453,50 +503,38 @@ function build(syscall, forVatID, cacheSize, vatPowers, vatParameters) {
     slotToVal.delete(promiseID);
   }
 
-  const ENABLE_PROMISE_ANALYSIS = true;
-
-  function retirePromiseIDIfEasy(promiseID, data) {
-    if (ENABLE_PROMISE_ANALYSIS) {
-      for (const slot of data.slots) {
-        const { type } = parseVatSlot(slot);
-        if (type === 'promise') {
-          lsdebug(
-            `Unable to retire ${promiseID} because slot ${slot} is a promise`,
-          );
-          return;
-        }
-      }
-    }
-    retirePromiseID(promiseID);
-  }
-
-  function thenResolve(promiseID) {
+  function thenHandler(p, promiseID, rejected) {
     insistVatType('promise', promiseID);
-    return res => {
-      harden(res);
-      lsdebug(`ls.thenResolve fired`, res);
-      const ser = m.serialize(res);
-      syscall.resolve([[promiseID, false, ser]]);
+    return value => {
+      knownResolutions.set(p, [rejected, value]);
+      harden(value);
+      lsdebug(`ls.thenHandler fired`, value);
+      const resolutions = resolutionCollector().forPromise(
+        promiseID,
+        rejected,
+        value,
+      );
+
+      syscall.resolve(resolutions);
+
       const pRec = importedPromisesByPromiseID.get(promiseID);
       if (pRec) {
-        pRec.resolve(res);
+        if (rejected) {
+          pRec.reject(value);
+        } else {
+          pRec.resolve(value);
+        }
       }
-      retirePromiseIDIfEasy(promiseID, ser);
+      retirePromiseID(promiseID);
     };
   }
 
-  function thenReject(promiseID) {
-    return rej => {
-      harden(rej);
-      lsdebug(`ls thenReject fired`, rej);
-      const ser = m.serialize(rej);
-      syscall.resolve([[promiseID, true, ser]]);
-      const pRec = importedPromisesByPromiseID.get(promiseID);
-      if (pRec) {
-        pRec.reject(rej);
-      }
-      retirePromiseIDIfEasy(promiseID, ser);
-    };
+  function thenResolve(p, promiseID) {
+    return thenHandler(p, promiseID, false);
+  }
+
+  function thenReject(p, promiseID) {
+    return thenHandler(p, promiseID, true);
   }
 
   function notifyOnePromise(promiseID, rejected, data) {

packages/SwingSet/src/kernel/vatTranslator.js

@@ -4,7 +4,6 @@ import { insistKernelType, parseKernelSlot } from './parseKernelSlots';
 import { insistVatType, parseVatSlot } from '../parseVatSlots';
 import { insistCapData } from '../capdata';
 import { kdebug, legibilizeMessageArgs, legibilizeValue } from './kdebug';
-import { deleteCListEntryIfEasy } from './cleanup';
 
 /*
  * Return a function that converts KernelDelivery objects into VatDelivery
@@ -223,6 +222,7 @@ function makeTranslateVatSyscallToKernelSyscall(vatID, kernelKeeper) {
 
   function translateResolve(vresolutions) {
     const kresolutions = [];
+    const kpidsResolved = [];
     let idx = 0;
     for (const resolution of vresolutions) {
       const [vpid, rejected, data] = resolution;
@@ -238,19 +238,9 @@ function makeTranslateVatSyscallToKernelSyscall(vatID, kernelKeeper) {
       );
       idx += 1;
       kresolutions.push([kpid, rejected, kernelData]);
-      deleteCListEntryIfEasy(
-        vatID,
-        vatKeeper,
-        kernelKeeper,
-        kpid,
-        vpid,
-        kernelData,
-      );
+      kpidsResolved.push(kpid);
     }
-    // XXX TODO Once we get rid of the "if easy" logic, the above deletions
-    // should be collected and then processed in a batch here after all the
-    // translation is done, e.g., something like:
-    // vatKeeper.deleteCListEntriesForKernelSlots(targets);
+    vatKeeper.deleteCListEntriesForKernelSlots(kpidsResolved);
     return harden(['resolve', vatID, kresolutions]);
   }
 

packages/SwingSet/src/vats/comms/clist-kernel.js

@@ -47,6 +47,10 @@ export function makeKernel(state, syscall, stateKit) {
       // promise is retired (to remember the resolution).
       assert(p, `how did I forget about ${vpid}`);
 
+      if (p.kernelAwaitingResolve) {
+        return vpid;
+      }
+
       if (p.resolved) {
         // The vpid might have been retired, in which case we must not use it
         // when speaking to the kernel. It will only be retired if 1: it

packages/SwingSet/src/vats/comms/delivery.js

@@ -33,6 +33,7 @@ export function makeDeliveryKit(state, syscall, transmit, clistKit, stateKit) {
     changeDeciderFromRemoteToComms,
     getPromiseSubscribers,
     markPromiseAsResolved,
+    markPromiseAsResolvedInKernel,
   } = stateKit;
 
   function mapDataToKernel(data) {
@@ -340,6 +341,10 @@ export function makeDeliveryKit(state, syscall, transmit, clistKit, stateKit) {
       // promiseTable reminds provideKernelForLocal to use a fresh VPID if we
       // ever reference it again in the future
     }
+    for (const resolution of resolutions) {
+      const [vpid] = resolution;
+      markPromiseAsResolvedInKernel(vpid);
+    }
   }
 
   function resolveToRemote(remoteID, resolutions) {

packages/SwingSet/src/vats/comms/state.js

@@ -244,12 +244,20 @@ export function makeStateKit(state) {
     assert(!p.resolved);
     insistCapData(resolution.data);
     p.resolved = true;
+    p.kernelAwaitingResolve = true;
     p.resolution = resolution;
     p.decider = undefined;
     p.subscribers = undefined;
     p.kernelIsSubscribed = undefined;
   }
 
+  function markPromiseAsResolvedInKernel(vpid) {
+    const p = state.promiseTable.get(vpid);
+    assert(p, `unknown ${vpid}`);
+    assert(p.resolved && p.kernelAwaitingResolve);
+    p.kernelAwaitingResolve = false;
+  }
+
   return harden({
     trackUnresolvedPromise,
     allocateUnresolvedPromise,
@@ -275,6 +283,7 @@ export function makeStateKit(state) {
 
     insistPromiseIsUnresolved,
     markPromiseAsResolved,
+    markPromiseAsResolvedInKernel,
 
     dumpState: () => dumpState(state),
   });

packages/SwingSet/test/message-patterns.js

@@ -895,6 +895,42 @@ export function buildPatterns(log) {
   out.a91 = ['carol got Pbert', 'hi bert'];
   test('a91');
 
+  // 100-series: test cross-referential promise resolutions
+  test('a100');
+  {
+    objA.a100 = async () => {
+      const apa = await E(b.bob).b100_1();
+      const apb = await E(b.bob).b100_2();
+      const pa = apa[0];
+      const pb = apb[0];
+      E(b.bob).b100_3([pa], [pb]);
+      try {
+        const pa2 = (await pa)[0];
+        const pb2 = (await pb)[0];
+        const pa3 = (await pa2)[0];
+        const pb3 = (await pb2)[0];
+        log(`${pb3 !== pa3}`);
+        log(`${pb3 === pa2}`);
+        log(`${pa3 === pb2}`);
+      } catch (e) {
+        log(`a100 await failed with ${e}`);
+      }
+    };
+    const p1 = makePromiseKit();
+    const p2 = makePromiseKit();
+    objB.b100_1 = () => {
+      return [p1.promise];
+    };
+    objB.b100_2 = () => {
+      return [p2.promise];
+    };
+    objB.b100_3 = (apa, apb) => {
+      p1.resolve(apb);
+      p2.resolve(apa);
+    };
+  }
+  out.a100 = ['true', 'true', 'true'];
+
   // TODO: kernel-allocated promise, either comms or kernel resolves it,
   // comms needs to send into kernel again