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

feat(asyncFlow): E support #9322

Draft
wants to merge 18 commits into
base: master
Choose a base branch
from
Draft

feat(asyncFlow): E support #9322

wants to merge 18 commits into from

Conversation

erights
Copy link
Member

@erights erights commented May 6, 2024

Staged on #9443
closes: #9299
refs: #9097 #9336 #9519

Description

To support guest uses of E to do eventual sends,

  • we make guest remotable wrappers of host remotables into remote presences. This avoids any need to distinguish between local host remotables vs host remote presences.
  • we make guest promise wrappers of host vows into pipelinable promises.

Not yet implemented

  • Both have a handler that will implement applyMethod to translate an eventual send on the guest wrapper into a corresponding eventual send to the host target, logging it as a checkSend, by analogy with a checkCall. Like a checkCall that immediately returns a promise, it will be followed immediately by a corresponding doReturn of the corresponding vow. Unlike checkCall/doReturn, there is no possible callback interleaving between a checkSend and its doReturn. Later, when the target vow has settled, that will cause a corresponding doFulfill or doReject. Unlike checkCall, the target of a checkSend can be either a remotable or a promise.

Security Considerations

just more surface area and more tricky things that might be wrong.

Scaling Considerations

should not be significantly different than the existing support for checkCall.

Documentation Considerations

one less restriction that needs to be documented. Once this PR works, it should "just work" in the sense of merely removing a surprise.

Testing Considerations

  • write tests

Upgrade Considerations

Prior to #9443 and this PR, a guest use of E(guestWrapper).foo() would, in a later turn, do a guest-side guestWrapper.foo() causing a checkCall/doReturn to get logged. This is not incorrect on its own terms. But we need not to commit to any such durable logs prior to this PR. With this PR, that same expression will immediately cause checkSend to get logged with an immediate doReturn of a promise that later settles with a doFulfill or doReject. With this change in the log, we expect we have durable logs we're willing to commit to.

  • We need to decide how the asyncFlow mechanism should treat such errors.
    • Are they in model -- where the not-yet-implemented error gets thrown back at the guest function?
    • Or are they failures suspending the guest in a Failed state, so a later upgrade of the asyncFunc implementation would allow such failed guests to "resume" without requiring any change to the guest?

@erights erights changed the base branch from master to markm-async-flow May 6, 2024 05:58
@erights erights self-assigned this May 6, 2024
@erights erights changed the title feat(asyncFlow): E support feat(asyncFlow): E support May 6, 2024
Copy link

cloudflare-pages bot commented May 6, 2024

Deploying agoric-sdk with  Cloudflare Pages  Cloudflare Pages

Latest commit: 290ded5
Status: ✅  Deploy successful!
Preview URL: https://86dc382d.agoric-sdk.pages.dev
Branch Preview URL: https://markm-asyncflow-e.agoric-sdk.pages.dev

View logs

@erights erights requested review from mhofman and michaelfig May 6, 2024 06:23
@erights erights added the asyncFlow related to membrane-based replay and upgrade of async functions label May 6, 2024
@erights erights force-pushed the markm-asyncFlow-E branch 2 times, most recently from e8e56d9 to 7575a6c Compare May 8, 2024 01:25
@erights erights force-pushed the markm-async-flow branch 2 times, most recently from 596304e to 4cb7ed6 Compare May 8, 2024 23:24
@erights erights changed the base branch from markm-async-flow to markm-zonify-vowTools May 8, 2024 23:29
Base automatically changed from markm-zonify-vowTools to markm-async-flow May 14, 2024 18:32
Base automatically changed from markm-async-flow to master May 19, 2024 22:08
@erights erights force-pushed the markm-asyncFlow-E branch 2 times, most recently from e9420ab to 98c12fb Compare May 24, 2024 03:29
Copy link
Member

@mhofman mhofman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First pass, but I'm confused about the handling of the result vow. I understand we have the logic about interpreter nesting, but I think it would be simpler to include the result vow in the checkSend log entry, and have a synthetic doReturn with no result (and ignore the outcome completely on replay because it either succeeds or panics, a throw outcome should not happen for a checkSend)

const hostPromise = optVerb
? E(hostTarget)[optVerb](...hostArgs)
: E(hostTarget)(...hostArgs);
resolver.resolve(hostPromise); // TODO does this always work?
Copy link
Member

@mhofman mhofman May 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah thinking about this, if hostTarget is a local object for which call returns a promise, we will fail during an upgrade. Unlike the checkCall case, we're not in a position to do any enforcment and fail unconditionally, unless we can somehow sniff the type of the hostTarget (if it has an eventual handler or not), and bypass the E call if it doesn't. If the hostTarget object does not return a promise, then the hostPromise will be fulfilled in the same crank, so it's safe.

: E(hostTarget)(...hostArgs);
resolver.resolve(hostPromise); // TODO does this always work?
} catch (hostProblem) {
throw Fail`internal: eventual send synchrously failed ${hostProblem}`;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since I didn't understand this right originally, let's add a comment: Since a well behaved eventual-send should not synchronously fail (it doesn't synchronously interact with the target), a synchronous throw does not represent a failure we should commit to, but instead is a panic.

Comment on lines +261 to +266
// TODO FIX BUG this is not quite right. When guestResultP is returned
// as the resolution of guestResultP, it create a visious cycle error.
const hostResultKit = makeVowKit();
bijection.init(guestReturnedP, hostResultKit.vow);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you clarify how this cycle could actually happen?

Comment on lines +263 to +266
const hostResultKit = makeVowKit();
bijection.init(guestReturnedP, hostResultKit.vow);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think if we're replaying we need to use the vow that was previously created for the send, no?

I also don't see how we're rewiring the guestReturnedP to the watched vow on replay.

Comment on lines +250 to +254
} catch (problem) {
throw panic(problem);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't the caller of performSend already panic on exceptions?

Comment on lines 352 to 357
const promise = new HandledPromise((res, rej, _resPres) => {
resolve = res;
reject = rej;
}, guestHandler);
Copy link
Member

@mhofman mhofman May 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI, makePromiseKit has some logic to sever references from the resolvers to the promise once resolved, but I think in this case all resolver users are well behaved and drop resolvers after usage, so we're good.

Comment on lines +308 to +312
case 'throw': {
throw outcome.problem;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this can ever happen, can it?

Comment on lines +231 to +234
? E(hostTarget)[optVerb](...hostArgs)
: E(hostTarget)(...hostArgs);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the case of host vows, we should be using the V helper instead of E

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After talking with @michaelfig we clarified that V is heap only, and does not queue sends durably. We need to structure the membrane handling of sends to vows such that they are only sent after the vow has settled. We can rely on the fact that the membrane has replayed (no need to durably store the pending sends, instead use the guest promise).

Base automatically changed from markm-asyncFlow-no-E to master June 10, 2024 17:03
mergify bot pushed a commit that referenced this pull request Jun 10, 2024
closes: #XXXX
refs: #9299 #9322

## Description

Prepare for #9322 by making any guest use of `E` until then cause an
error. We expect that it might be a while before #9322 is ready for
review. By merging this PR soon, we prevent any guest code or logs that
would commit us to an incompat way of handling `E`.

### Security Considerations

none
### Scaling Considerations

none
### Documentation Considerations

Should document that guests cannot invoke host vows or remotables with
`E` until #9322 , which won't happen immediately.
### Testing Considerations

- [x] need to test what kind of error state this goes into. Should be a
panic, so that an upgrade can unblock guest execution that got stuck
trying to `E`.
### Upgrade Considerations

The point. By making such use of `E` an error now, we ensure that #9322
can proceed without causing any compat problem with committed durable
state.

Co-authored-by: Mathieu Hofman <86499+mhofman@users.noreply.github.com>
0xpatrickdev added a commit that referenced this pull request Jun 21, 2024
 * Perform remote calls to agoricNames in membrane-friendly way. This is an
 * interim approach until #9541,
 * #9322, or
 * #9519
0xpatrickdev added a commit that referenced this pull request Jun 21, 2024
 * Perform remote calls to agoricNames in membrane-friendly way. This is an
 * interim approach until #9541,
 * #9322, or
 * #9519
0xpatrickdev added a commit that referenced this pull request Jun 25, 2024
 * Perform remote calls to agoricNames in membrane-friendly way. This is an
 * interim approach until #9541,
 * #9322, or
 * #9519
0xpatrickdev added a commit that referenced this pull request Jun 25, 2024
 * Perform remote calls to agoricNames in membrane-friendly way. This is an
 * interim approach until #9541,
 * #9322, or
 * #9519
mergify bot added a commit that referenced this pull request Jun 25, 2024
refs: #9449

## Description

 Perform remote calls to agoricNames in membrane-friendly way. This is an interim approach until #9541, #9322, or #9519.
mergify bot pushed a commit that referenced this pull request Jun 28, 2024
closes: #XXXX
refs: #9322, #9299 #9443

## Description

PR #9322 is supposed to provide production quality support for asyncFlow guest functions to use `E`. It is being reviewed for that goal, and will not be merged until we think it meets that bar. However, we need to start integration testing of asyncFlow with orchestration, to spot mismatched assumptions we may have missed. For this purpose, we do not immediately need production quality `E` support. That is the purpose of this PR. It starts as a copy of the code from #9322 but need only be evaluated as adequate for these stopgap purposes before being merged. This PR does *NOT* claim to f-i-x #9299 , leaving that job to remain with #9322 

Even though the requirements on this PR are so much lighter, reviewers should still look at the unresolved conversations on #9322 and determine if any of those need to first be solved even in this PR.

### Security Considerations

When merging stopgap code to master, there is always the danger that it might be used as if it production code. We need to remember not to do so, instead waiting for #9322 to do the job for real.

### Scaling Considerations
none
### Documentation Considerations
just as this stopgap unblocks integration testing, it also likely unblocks documenting how to use asyncFlow, both in general and for orchestration.
### Testing Considerations
As a stopgap, this PR does not need the rigorous testing that #9322 should have.
### Upgrade Considerations
We need to not use this stopgap for production purposes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
asyncFlow related to membrane-based replay and upgrade of async functions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

asyncFlow should directly support E via promise handlers
2 participants