Handle incoming receiverAnswer cap descriptors.
#209
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This only actually gets used in sendCap(), but there we have a reference to the corresponding client, so we can just call client.State() instead of carrying around this parallel slice everywhere.
Useful to avoid needing to expand the if/else in-line everywhere.
...in preparation for handling incoming receiverAnswer caps.
Needs testing, and I'm not sure what the right value is to return for local; need to think about it.
...which fails; need to debug.
I think I initially put this in to try to work around deadlock issues with net.Pipe(), but that didn't work anyway, and is unnecessary now that the test uses socketpair().
There is no field resultsCapTable, only resultCapTable.
This solves an issue I discovered where sendCap() does a linear search on the exports table for each sent capability. After a long discussion with Louis over matrix, we concluded that adding a metadata object for clients was the best solution; there's a general design awkwardness right now where the rpc system needs to be able to inspect the client in various ways, but Client is defined in the main package and tries to encapsulate its implementation details. To some degree we are able to use Brand(), but this gets awkward quickly, because: 1. ClientHook is an openly implementable interface, so we don't know all the posibilities in the rpc code. 2. As I've been working on supporting receiverAnswer, I've found pinning down all the implementations I defined in the library that need to be worried about is challenging. 3. Some of these implementations aren't even exported, or just return nil from Brand(). The Metadata object lets the rpc system manage the bookkeeping information independently of the internals of a client. I expect to use this for more in the future, but for now this lets us avoid the linear search by just storing the export id for each connection in the client's metadata.
This improves type safety at call sites. It's actually more code right now, but I am working on a branch where I use these elsewhere.
At least one case where we should return local = true. Need to think through the logic for everything else.
I have convinced myself that this is necessary, though note that this is not a complete solution; I need to work out how this interacts with embargo stuff everywhere else.
Rather than having recvCap deal with this directly, we now have a separate routine that just checks if a client is local, which we invoke higher up in the chain. This means we're not trying to figure this out based on the cap descriptor, which means (1) fewer cases, and (2), in the case of a cap descriptor that denotes a promise or answer, we will be able to recursively call this on the target -- whose original CapDescriptor may not be known. This introduces one minor functional change: ErrorClients are now treated as local. This means we'll set up embargos for them, which we don't *need* to do, but it shouldn't be a problem; calling methods on errors is not a case that is terribly sensitive to the extra round trip. We could try to rework ErrorClient to avoid this, but it doesn't seem worth the trouble.
...and update our changes to recvCap to match.
|
Ok, I think the logic for local in recvCap is right now, though I still need to write some more tests. Relatedly, this also includes #212, which should be merged first. |
This one projects off a call, rather than a bootstrap
zenhack
changed the title
receiverAnswer cap descriptors.receiverAnswer cap descriptors.
|
Alright, added another test, which projects on calls. I consider this ready to go, but these should be reviewed & merged first: |
lthibault
previously approved these changes
Jan 3, 2022
rpc/answer.go
Outdated
| ans.pcall = nil | ||
| ans.flags |= resultsReady | ||
| // XXX: do we need to check if we've received a finish? if so, the |
There was a problem hiding this comment.
The XXX suggests that this is a piece of missing implementation. Is that (still) the case?
There was a problem hiding this comment.
Good catch, fixed. Also fixed merge conflicts.
I'd meant to fill this in and forgot; thanks to Louis for spotting.
lthibault
approved these changes
Jan 3, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Marking this as a draft, still TODO:
Note that this includes the changes from #207 and #208, so the diff is bigger than it should be.