Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

async buildRootObject #5246

Closed
warner opened this issue Apr 29, 2022 · 4 comments · Fixed by #5250
Closed

async buildRootObject #5246

warner opened this issue Apr 29, 2022 · 4 comments · Fixed by #5250
Assignees
Labels
enhancement New feature or request SwingSet package: SwingSet

Comments

@warner
Copy link
Member

warner commented Apr 29, 2022

What is the Problem Being Solved?

#3272 turns out to have a surprising requirement: we need to do importBundle (which is async, for deep reasons) from inside buildRootObject (which is sync). This comes from ZCF needing to import the new version of the contract, then invoke the contract method that re-establishes behavior for the durable Kinds, all of which needs to happen before the end of the buildRootObject delivery (so it can execute the normal deliveries that will immediately follow).

D(bundlecap).getBundle() -> bundle is synchronous, for exactly this reason, but I forgot that importBundle is async.

Description of the Design

It looks like making liveslots tolerate an async buildRootObject is the best way to deal with this. The tricky part is the error handling, in particular what to do if the user-provided buildRootObject function returns a Promise that never resolves. We must prevent userspace from hanging the supervisor.

The tail end of startVat currently does:

    const rootObject = buildRootObject(harden(vpow), harden(vatParameters), baggage);
    assert.equal(passStyleOf(rootObject), 'remotable', X`buildRootObject() for vat ${forVatID} returned ${rootObject}, which is not Far`);
    assert(getInterfaceOf(rootObject) !== undefined, X`buildRootObject() for vat ${forVatID} returned ${rootObject} with no interface`);
    // Need to load watched promises *after* buildRootObject() so that handler kindIDs
    // have a chance to be reassociated with their handlers.
    watchedPromiseManager.loadWatchedPromiseTable();

    const rootSlot = makeVatSlot('object', true, BigInt(0));
    valToSlot.set(rootObject, rootSlot);
    slotToVal.set(rootSlot, new WeakRef(rootObject));
    retainExportedVref(rootSlot);
    // we do not use vreffedObjectRegistry for root objects

    vom.insistAllDurableKindsReconnected();
  }

and the code which calls it is basically:

async function dispatch(delivery) {
  const p = Promise.resolve(delivery).then(startVat);
  return waitUntilQuiescent().then(() => p);
}

I'm thinking the trick will be to connect the buildRootObject promise to a function that finishes the startVat process, then sets a flag. We return a separate promise from startVat. I need a way for that second promise to reject if the flag isn't set at the time waitUntilQuiescent fires. That probably means introducing a callback into the () => p portion, to query the flag.

Security Considerations

Main requirement is to not allow an unresolved userspace promise to hang the worker (and kernel).

Test Plan

unit tests

@warner warner added enhancement New feature or request SwingSet package: SwingSet labels Apr 29, 2022
@warner warner self-assigned this Apr 29, 2022
@FUDCo
Copy link
Contributor

FUDCo commented Apr 29, 2022

Is it considered a valid practice to inspect the return value from a function to see if it's a Promise or not? Or even not look but just fail if a Promise comes back. Because I'd like us to be able to continue using fully synchronous buildRootObject in contexts (basically, tests) that currently assume sync. Otherwise we'll have to update a lot of stuff.

@mhofman
Copy link
Member

mhofman commented Apr 29, 2022

It's frowned up. The main problem is dealing with thenable objects, especially evil ones.

@FUDCo
Copy link
Contributor

FUDCo commented Apr 29, 2022

Our tests are not going to be returning evil thenables.

@warner
Copy link
Member Author

warner commented Apr 29, 2022

I think we can have startVat just do an await upon the return value of buildRootObject like before, so returning a non-Promise should be trivial. The rule will be that buildRootObject must either return the root object, or a Promise which resolves (within the same crank) to the root object, and that all Kinds must be re-established before that Promise returns. If the Promise does not return before end-of-crank, startVat declares failure, same as if buildRootObject throws or returns a rejected Promise.

warner added a commit that referenced this issue Apr 29, 2022
New (or upgraded) vats are defined by a user-provided
`buildRootObject` function. This function has two responsibilities:

* provide a root object
* use `defineDurableKind` to bind behavior to any pre-existing durable
  kinds

Previously, `buildRootObject` was given exactly one "turn" to fulfill
these responsibilities: it was required to return the root object
synchronously. If it returned a Promise, that was rejected as an
error.

This requirement is at odds with the needs for updated ZCF vats to
evaluate their contract code and interact with it (giving the contract
a chance to bind their own Kind behavior). The `importBundle` function
we use to load contracts is async. However, it is still "prompt": it
needs no IO, nor any messages leaving the vat. So it will still
complete within the *crank*, just not synchronously within the turn.

So this commit changes liveslots to allow `buildRootObject` to return
a Promise for the root object. The requirement, however, is that this
Promise must resolve by the end of the initial crank (before
`waitUntilQuiescent` / `setImmediate` determines that the promise
queue is empty). If the Promise is not fulfilled by then, the vat
creation/update is considered broken.

closes #5246
warner added a commit that referenced this issue Apr 29, 2022
New (or upgraded) vats are defined by a user-provided
`buildRootObject` function. This function has two responsibilities:

* provide a root object
* use `defineDurableKind` to bind behavior to any pre-existing durable
  kinds

Previously, `buildRootObject` was given exactly one "turn" to fulfill
these responsibilities: it was required to return the root object
synchronously. If it returned a Promise, that was rejected as an
error.

This requirement is at odds with the needs for updated ZCF vats to
evaluate their contract code and interact with it (giving the contract
a chance to bind their own Kind behavior). The `importBundle` function
we use to load contracts is async. However, it is still "prompt": it
needs no IO, nor any messages leaving the vat. So it will still
complete within the *crank*, just not synchronously within the turn.

So this commit changes liveslots to allow `buildRootObject` to return
a Promise for the root object. The requirement, however, is that this
Promise must resolve by the end of the initial crank (before
`waitUntilQuiescent` / `setImmediate` determines that the promise
queue is empty). If the Promise is not fulfilled by then, the vat
creation/update is considered broken.

closes #5246
@mergify mergify bot closed this as completed in #5250 Apr 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request SwingSet package: SwingSet
Projects
None yet
3 participants