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

Support arbitrary client metadata, avoid linear seach in sendCap. #208

Merged
merged 9 commits into from
Jan 3, 2022

Conversation

zenhack
Copy link
Contributor

@zenhack zenhack commented Jan 1, 2022

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 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.
capability.go Show resolved Hide resolved
lthibault
lthibault previously approved these changes Jan 3, 2022
Copy link
Collaborator

@lthibault lthibault left a comment

Choose a reason for hiding this comment

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

Approving this because it looks correct. Nevertheless, I feel moderately strongly that the two readability issues I've flagged should be addressed before merging. Your call.

capability.go Outdated Show resolved Hide resolved
capability.go Show resolved Hide resolved
rpc/export.go Show resolved Hide resolved
@lthibault
Copy link
Collaborator

Could this check in sendCap be refactored to use isLocalClient from #212 ?

That would be a huge readability win. The above snippet is quite jarring every time I look at it, despite the fact that the domain logic it represents is dead-simple.

@zenhack
Copy link
Contributor Author

zenhack commented Jan 3, 2022

Could this check in sendCap be refactored to use isLocalClient from #212 ?

I assume you meant to link to a specific line? The link just points to the whole diff, so I'm not sure which check you mean.

@lthibault
Copy link
Collaborator

Looks like it was merged, so no longer appearing in the diff: https://github.com/capnproto/go-capnproto2/blob/8c878e053da7da0aa3d7c46f9c60c1338efaa84a/rpc/export.go#L88-L93

However, I just saw that we actually use the concrete *importClient, so never mind.

Per discussion with Louis. The wording of Metadata's doc comment has
been updated to also suggest that it is ok to copy Metadata before use.
lthibault
lthibault previously approved these changes Jan 3, 2022
Copy link
Collaborator

@lthibault lthibault left a comment

Choose a reason for hiding this comment

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

Cheers, much nicer to look at :)

@lthibault lthibault merged commit 8e5432d into capnproto:main Jan 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants