Implement durable promise watchers #5130

FUDCo · 2022-04-17T08:02:17Z

This provides two new functions on VatData, per the design hashed out in #5006 as modified by the experience of implementing it:

providePromiseWatcher(kindHandle, fulfillHandler, rejectHandler)
watchPromise(p, watcher, ...args)

where:
kindHandle is a kind handle such as is returned by makeKindHandle
fulfillHandler is a function of the form (value, ...args) => void
rejectHandler is a function of the form (err, ...args) => void
p is a promise
watcher is a watcher object (explained below)
...args are arbitrary arguments that will be passed to the watcher when the promise is fulfilled or rejected

providePromiseWatcher returns a promise watcher object for use in watchPromise. The two handler arguments are functions that will handle promise fulfillments and rejections respectively.

The watcher that is passed to watchPromise can be either a watcher object as returned by providePromiseWatcher or any durable object (durable object facet) that implements one or both of the methods:

onFulfilled(context, value, ...args)
onRejected(context, err, ...args)

watchPromise can watch any promise and this watching will survive shutdown and restart (upgrade) of its vat, though of course when the vat restarts, promises for which the vat itself was the decider will all have been rejected by the act of shutdown.

FUDCo · 2022-04-17T08:19:39Z

Turns out this needs TS type defs in the vat-data package. I have no clue how to do this, so help would appreciated on that score.

mhofman

Preliminary review of the implementation.

The blocker right now is the mishandling of throwing handlers, and the synchronous invocation of these during init for disconnected promises.

I am also concerned that this approach has an unnecessary RAM cost for imported promises.

Rest is mostly nits and clarifications.

two new functions on VatData
providePromiseWatcher(kindHandle, fulfillHandler, rejectHandler)

I was hoping the providePromiseWatcher could live in user-land (e.g. in @agoric/vat-data), but I suppose we need a mechanism to retrieve the singleton instance on upgrade? I kinda wish vat-data could keep promiseWatcherByKindTable in the baggage, but I suppose we still don't have a good mechanism to initialize such contract side helpers besides putting stuff on the global object when creating the compartment.

any durable object (durable object facet) that implements one or both of the methods

My reading of the code is that both are not required. However I'm not sure if we should test the object. We could imagine a future where the watcher just needs to be durable compatible, and the actual watcher is in fact in a remote vat, in which case testing can only happen for local watchers.

Turns out this needs TS type defs in the vat-data package. I have no clue how to do this, so help would appreciated on that score.

I can give it a shot tomorrow.

packages/SwingSet/src/kernel/kernel.js

packages/SwingSet/src/liveslots/watchedPromises.js

mhofman · 2022-04-17T18:21:02Z

packages/SwingSet/src/liveslots/watchedPromises.js

+      }
+    }
+
+    p.then(


FYI, since this can be a user provided promise, this can technically throw or synchronously re-enter into liveslots, but from what I gather, this is safe in this case. If we could add a note however, that would be great.

packages/SwingSet/src/liveslots/watchedPromises.js

mhofman

Thanks for addressing my initial concerns. Will review the tests later this afternoon.

packages/SwingSet/src/liveslots/watchedPromises.js

Blocking issues addressed

packages/SwingSet/src/liveslots/watchedPromises.js

packages/SwingSet/test/promise-watcher/bootstrap-promise-watcher.js

packages/SwingSet/src/liveslots/watchedPromises.js

mhofman

A few more nits, but looking good overall!

mhofman · 2022-04-21T02:51:43Z

packages/SwingSet/src/liveslots/stop-vat.js

@@ -41,7 +41,7 @@ function rejectAllPromises({ m, deciderVPIDs, syscall }) {
  // and attached a .then to it), and stopVat() must not allow
  // userspace to gain agency.

-  const rejectCapData = m.serialize('vat upgraded');
+  const rejectCapData = m.serialize(Error('vat upgraded'));


I vaguely remember a discussion about the upgrade message being differentiable by subscribers because it was a string instead of an Error like when a vat is terminated for failure. @warner

I was getting complaints from eslint, so assumed it was a mistake.

We should definitely be consistent (among all places that announce vat-upgrade). I think I've been slightly conditioned to not use Errors because the marshal serialization is so.. enthusiastic.. about announcing them to the console (multiple times), and stop-vat.js is new so I might have used a string while writing (and running) the tests.

We do need Notifier subscribers to be able to distinguish between vat-upgrade (meaning they should try again) and vat-failure (meaning they should stop). We could either use string-vs-Error for that, or have clients distinguish between err.message (which makes us more dependent upon the actual strings, and reduces our wiggle room for changing them in the future).

@erights as keeper of Notifier, do you think we should make vat-upgrade reject promises with a string, so you can change the Notifier client-side pattern to re-try unless it gets rejected with an Error? Or is there some different/better pattern we should use?

I'll open an issue. I feel uncomfortable discriminating based on string vs error instance like this for notifiers

@erights @warner @gibson042 can you help me triage #5185
Not sure which label it should fit under.

packages/SwingSet/src/liveslots/watchedPromises.js

mhofman · 2022-04-21T02:57:02Z

packages/SwingSet/src/liveslots/watchedPromises.js

+    // The following wrapping defers setting up the promise watcher itself to a
+    // later turn so that if the promise to be watched was the return value from
+    // a preceding eventual message send, then the assignment of a vpid to that
+    // promise, which happens in a turn after the initiation of the send, will
+    // have happened by the time the code below executes, and thus when we call
+    // `convertValToSlot` on the promise here we'll get back the vpid that was
+    // assigned rather than generating a new one that nobody knows about.
+    Promise.resolve().then(() => {


Ooof that feels brittle. Any way to pass an option to convertValToSlot that there must already be a slot allocated?

Yeah, we're a little nervous too. The problem is that there isn't already a slot allocated so convertValToSlot allocates one, but then the message send code allocates another one (in a way that's entirely different). There's magic voodoo promise plumbing in the middle that one interferes with at one's peril.

But by the time Promise.resolve() reaction runs and calls convertValToSlot, the goal is that it would be allocated then, or did I misunderstood? If that's the case, there is no way to assert it?

Ah, I misunderstood what you were saying. Yes, by the time Promise.resolve() runs everything is OK -- so I gather you're suggesting an option to assert that everything actually is OK at that point?

Aye, just in case something (e.g. the promise implementation) changes from under our feet in some conditions, and our assumption no longer holds

Well, if there is a slotToVal entry then convertValToSlot will just use it, so in that case there's not actually extra machinery in convertValToSlot that we're leaning on here. And in that case we might just call requiredValForSlot. But I'm pretty sure this would not do the right thing in the case where the vpid is not being generated by a message send, in which case there might well not be an existing slotToVal entry and we really do need convertValToSlot to do all the work.

And never mind that last comment. I'm always getting convertSlotToVal and convertValToSlot mixed up, because we also have getValForSlot and getSlotForVal where the pattern of the names is reversed. This confusion has been on our hitlist for a while now.

So let me try again: in the case where the vpid is being generated by a message send, we need the deferred execution that Promise.resolve()... provides so that convertValToSlot will see the vpid that the message send produced. However, in the case where it's not a message send, we really do need convertSlotToVal to generate the vpid. In the latter case we don't need the deferral, but at the point where watchPromise is called we don't know which case we're in, and the deferral is harmless in the case where we don't actually need it.

That makes sense. I guess the only approach would be to somehow synchronously mark the promise returned by the send, and check for it here, but that seems to involved. I'm a little surprised that the promise used as result of the send is processed by convertValToSlot with a delay. Isn't there other cases where this would trip things, like using such a promise as a send argument immediately:

const rp = E(alice).foo(); E(bob).bar(rp);

packages/SwingSet/test/promise-watcher/unhandledRejectionDetector.js

warner

One minor change, plus we need an answer from @erights about whether vat upgraded should be an Error or a string. After that we're good to land.

warner · 2022-04-21T18:08:25Z

packages/SwingSet/src/liveslots/watchedPromises.js

+            if (watcher.onRejected) {
+              watcher.onRejected(Error('vat upgraded'), ...args);
+            } else {
+              assert.fail('vat upgraded');


Let's build a shared Error('vat upgraded') at like line 99, and then change this assert.fail into throw err. Using an assert makes it read like this is a "shouldn't happen" case, where actually what we're doing is allowing the unhandled rejection handler to observe the unhandled rejection.

One minor change, plus we need an answer from @erights about whether vat upgraded should be an Error or a string. After that we're good to land.

Let's try a string. I bet that non-error rejections are only lightly tested at this point, so it wouldn't surprise me if it revealed some such bugs. Which would be a good thing.

Unless there are surprises, I think I like the string idea. Or at least something that is obviously not an Error.

Well OK then.

@erights I assume you mean to comment on https://github.com/Agoric/agoric-sdk/pull/5130/files#r855445250 ?

For here, an error instance is appropriate. However, we probably should use a shared frozen error built once outside the loop

Two additional thoughts: the original reason I switched it from a string to an Error was that eslint whines if you throw a string. Obviously this can be suppressed with a disabling comment, bit it seems kinda hinky. Also, I think I'm unclear what the benefit of using a string in this particular case is supposed to be. I understand flushing out bugs, but is this the right place to be doing that?

Oy. My preference would be to use Error here (and everywhere else there's a throw, but that would be a separate task). And file a ticket to get rid of the annoying behavior of marshal spewing to the log any time it serializes an Error, which i hate, hate, hate.

@erights I guess I'm asking you to reconsider your call above.

Sure. Let's have a conversation where we jointly reconsider. Schedule something?

@erights and I discussed this and concluded that signaling vat upgrade with a string is preferred to using Error (even better would be to have an exceptional condition indicator object analogous to Error but which is not Error, but we don't have the time to prioritize that right now). Whatever entity is attending to the vat upgrade event should not be discriminating on the basis of whether the thing thrown was string or an Error however, but should specifically be looking for the particular string (in this case, 'vat upgraded') that indicates the condition of interest.

On that basis I'm going to revise this PR to revert to using a string.

Can we please make it a const at the top level somewhere, and document this string must not change? Then you can throw the const everywhere, and shouldn't have to disable eslint.

warner

great!

warner · 2022-04-21T22:21:20Z

packages/SwingSet/src/liveslots/liveslots.js

@@ -710,6 +723,7 @@ function build(
        // this is a new import value
        val = makeImportedPresence(slot, iface);
      } else if (type === 'promise') {
+        // XXX note: this assert is the same as the one 5 lines above and therefor pointless


agreed, that's vestigal, we can remove it the next time we swing through this neighborhood

warner · 2022-04-21T22:29:03Z

packages/SwingSet/test/promise-watcher/test-promise-watcher.js

+    'rp4-pw rejected rerrafter version v2 via watcher []',
+    'rp4-dk rejected rerrafter version v2 via VDO',
+  ]);
+  if (os.platform() === 'darwin') {


FWIW, I was able to remove this check on my pseudo-linux box (macOS running a linux kernel under Docker using Node v16.14.0) and also a real linux box (also with Node v16.14.0). What version of Node are you running under macOS locally?

I'm not concerned deeply about testing the unhandled rejection thing, but the os.platform check is sorta awkward, so after this lands, let's see if we can get rid of it. Maybe we just need to be running a newer version of Node in CI.

warner · 2022-04-21T22:35:48Z

Oh see if you can add that const vatUpgradedMessage = 'vat upgraded'; // part of the swingset API thing @mhofman recommended.

Closes #5006

FUDCo added enhancement New feature or request SwingSet package: SwingSet labels Apr 17, 2022

FUDCo requested review from warner and mhofman April 17, 2022 08:02

FUDCo self-assigned this Apr 17, 2022

FUDCo force-pushed the 5006-durable-promise-watchers branch 2 times, most recently from 9737388 to 68cbfa0 Compare April 17, 2022 18:00

mhofman previously requested changes Apr 17, 2022

View reviewed changes

FUDCo requested a review from mhofman April 19, 2022 07:47

FUDCo force-pushed the 5006-durable-promise-watchers branch from df8d785 to a76aab5 Compare April 19, 2022 07:48

mhofman reviewed Apr 19, 2022

View reviewed changes

packages/SwingSet/src/liveslots/watchedPromises.js Outdated Show resolved Hide resolved

packages/SwingSet/src/liveslots/watchedPromises.js Outdated Show resolved Hide resolved

mhofman self-requested a review April 19, 2022 19:03

warner requested changes Apr 19, 2022

View reviewed changes

warner reviewed Apr 19, 2022

View reviewed changes

packages/SwingSet/src/liveslots/watchedPromises.js Outdated Show resolved Hide resolved

FUDCo force-pushed the 5006-durable-promise-watchers branch from a76aab5 to b0ac984 Compare April 21, 2022 02:38

FUDCo requested a review from warner April 21, 2022 02:43

FUDCo force-pushed the 5006-durable-promise-watchers branch from b0ac984 to b27c41a Compare April 21, 2022 02:48

mhofman approved these changes Apr 21, 2022

View reviewed changes

FUDCo force-pushed the 5006-durable-promise-watchers branch 3 times, most recently from 73edb3a to 67c5aba Compare April 21, 2022 07:59

warner requested changes Apr 21, 2022

View reviewed changes

FUDCo force-pushed the 5006-durable-promise-watchers branch 2 times, most recently from 7b281ca to 0f77f3e Compare April 21, 2022 21:58

warner approved these changes Apr 21, 2022

View reviewed changes

feat: implement durable promise watchers

ce55851

Closes #5006

FUDCo force-pushed the 5006-durable-promise-watchers branch from 0f77f3e to ce55851 Compare April 21, 2022 23:23

FUDCo added the automerge:rebase Automatically rebase updates, then merge label Apr 21, 2022

mergify bot merged commit a1784de into master Apr 21, 2022

mergify bot deleted the 5006-durable-promise-watchers branch April 21, 2022 23:38

mhofman mentioned this pull request Apr 22, 2022

a result promise being included in the args of the same message might cause problems #5189

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement durable promise watchers #5130

Implement durable promise watchers #5130

FUDCo commented Apr 17, 2022

FUDCo commented Apr 17, 2022

mhofman left a comment

mhofman Apr 17, 2022

mhofman left a comment

mhofman left a comment

mhofman Apr 21, 2022

FUDCo Apr 21, 2022

warner Apr 21, 2022

mhofman Apr 21, 2022

mhofman Apr 21, 2022

mhofman Apr 21, 2022

FUDCo Apr 21, 2022

mhofman Apr 21, 2022

FUDCo Apr 21, 2022

mhofman Apr 21, 2022

FUDCo Apr 21, 2022

FUDCo Apr 21, 2022

FUDCo Apr 21, 2022

mhofman Apr 21, 2022 •

edited

Loading

warner left a comment

warner Apr 21, 2022

erights Apr 21, 2022

FUDCo Apr 21, 2022

mhofman Apr 21, 2022

FUDCo Apr 21, 2022

FUDCo Apr 21, 2022

FUDCo Apr 21, 2022

erights Apr 21, 2022

FUDCo Apr 21, 2022

mhofman Apr 21, 2022

warner left a comment

warner Apr 21, 2022

warner Apr 21, 2022

warner commented Apr 21, 2022

+                    }
+                  }
+                  p.then(

Implement durable promise watchers #5130

Implement durable promise watchers #5130

Conversation

FUDCo commented Apr 17, 2022

FUDCo commented Apr 17, 2022

mhofman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhofman left a comment

Choose a reason for hiding this comment

mhofman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhofman Apr 21, 2022 • edited Loading

Choose a reason for hiding this comment

warner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

warner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

warner commented Apr 21, 2022

mhofman Apr 21, 2022 •

edited

Loading