Unify and pluralize promise resolution syscalls #2242
Conversation
FUDCo
force-pushed
the
promise-gc-phase3-step1
branch
from
f0c402a
to
222d99a
Compare
| ); | ||
| idx += 1; | ||
| kresolutions.push([kpid, rejected, kernelData]); | ||
| deleteCListEntryIfEasy( |
There was a problem hiding this comment.
I'm pretty sure that we need to hold off deleting any clist entries until we've finished translating all of them (the same pattern we're using in the kernel-to-vat direction, on both the kernel side and the vat side).
Oh, maybe you're planning to change this in the next PR? It'll become an unconditional deleteCListEntry, rather than being limited to the "easy" cases, right?
| markPromiseAsResolved(vpid, resolution); | ||
| function handleResolutions(resolutions) { | ||
| const [[primaryVpid]] = resolutions; | ||
| const { subscribers, kernelIsSubscribed } = getPromiseSubscribers( |
There was a problem hiding this comment.
This feels wonky.. I think we'll need to make sure subscribers of all the promises are notified, not just the subscribers of the first one. I believe it's possible for one of the secondary promises in the batch to be one that the vat already knows about:
- send P1 (unresolved) out through comms vat to remote A
- send P2 (unresolved) out through comms vat to remote B
- resolve P1 to data that contains P2
- now the
notifyevent is on the run-queue, but has not yet reached the comms vat
- now the
- resolve P2 to something
- now there are two
notifyevents on the run-queue
- now there are two
- the
notify(comms-vat, P1)event reaches the top- the kernel creates a batch with P1 as the first promise, and P2 as the second
- comms needs to send a two-promise batch (P1+P2) to remote A
- comms needs to send a one-promise batch (just P2) to remote B
- comms retires the kernel-facing vrefs for both P1 and P2
- the
notify(comms-vat, P2)event reaches the top- kernel ignores it because the P2 c-list entry was already retired
Figuring out what batches to create seems non-trivial, though, let's talk this through. I imagine this PR is good to land for now, but we'll need to examine this more closely and refine it in a later PR to make sure we're getting all the cases right.
There was a problem hiding this comment.
That sounds like a plausible argument, but I think that would explode into a bunch of very complicated mechanism which I'm not sure it is needed here. Keep in mind that primaryVpid is the root of a reference graph and that the comms vat is a simple pass through. So when notify notifies a bundle, that bundle travels as a unit through the network to the recipients; all the resolved promises in the bundle are in the same place.
If my thinking here is wrong, then we may have a more complicated mess on our hands.
There was a problem hiding this comment.
I have filed ticket #2249 to make sure we don't lose track of this.
| notifyOnePromise(vpid, vp.rejected, vp.data, willBeResolved); | ||
| } | ||
| // XXX question: do we need to call retirePromiseIDIfEasy (or some special | ||
| // comms vat version of it) here? |
There was a problem hiding this comment.
not here, because it needs to be done after the kernel-facing vrefs have been translated into comms-vat-local lrefs
in fact it might make sense to have resolveFromKernel be responsible for accumulating the willBeResolved list, and notify() becomes a no-op. hm.
FUDCo
force-pushed
the
promise-gc-phase3-step1
branch
from
222d99a
to
0b6b995
Compare
These changes lay the foundation for vat->kernel side of aggregate promise resolution, which is the logical dual of the tasks outlined in #2065. These changes provide a single, multi-promise resolution call that can be used to atomically resolve a promise and notify the kernel of any dependent promise resolutions.