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

Implement this_client() #456

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

ranfdev
Copy link

@ranfdev ranfdev commented Nov 27, 2023

fixes #87.

One question though: this currently works well for a struct implementing a single interface.
If a single struct implements multiple interfaces, there will be multiple this_client implementations, one for each interface.
I'm not sure how this is going to handle multiple interfaces...

For example:

struct Calculator{}
impl calculator_add::Server for Calculator {
  fn add(&mut self, params, results) {...} 
}
impl calculator_sub::Server for Calculator {
  fn sub(&mut self, params, results) {
    let client = calculator_sub::Server::this_client(self); // Works perfectly.
    let client = calculator_add::Server::this_client(self); // OUCH, we are trying to get a `calculator_add::Client` from a `calculator_sub::Client`.
  } 
}

If the two interfaces share the same ClientHook anyway, this is fine. Do they? Else, it's going to be a problem.


// We need to provide a way to get the corresponding ClientHook of a Server.
// Passing the ClientHook inside `Params` would require a lot of changes, breaking compatiblity
// with existing code and ruining the developer experience, because `Params` would end up containing more generics.
Copy link
Member

Choose a reason for hiding this comment

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

What if this_client() returned an untyped capability -- perhaps just a Box<dyn ClientHook>? That would not require any more generics, would it?
(It looks to me like that's how capnproto-c++ implements thisCap().)

Copy link
Author

Choose a reason for hiding this comment

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

Having a params.this_client() returning Box<dyn ClientHook> would be possible but it has the following drawbacks:

  • the user needs to manually cast the client.
  • params is passed by value, and can be moved around and stored eventually for a 'static lifetime. That means params would need to own a copy of the current client, requiring a Box allocation for each method call. Unless we store a Weak reference to the ClientHook, but then params.this_client() needs to return an Option<T>, because the params we own may reference a dead ClientHook.

Returning an Option<Box<dyn ClientHook>> isn't great compared to a fully typed calculator::op_add::Client

Copy link
Author

@ranfdev ranfdev Nov 29, 2023

Choose a reason for hiding this comment

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

An alternative API which doesn't require unsafe nor a static variable is
params.client_for(self). (Where self is actually &mut self, because we are inside a method call).

By requiring &mut self as a parameter, we restrict the ability to get a client while we are inside the method call, and we also have the type information required to return a typed Client.

EDIT: no, this API doesn't have enough type information to return a typed client... But at least it can return directly a Box<dyn ClientHook> and doesn't need to return an Option<Box<dyn ClientHook>>

@dwrensha
Copy link
Member

I implemented an alternative in #473.

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.

[RPC] add way to get a self-reference from within a method call
2 participants