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

Creating a context for each call is tedious #310

Open
garious opened this issue Jul 16, 2020 · 4 comments
Open

Creating a context for each call is tedious #310

garious opened this issue Jul 16, 2020 · 4 comments

Comments

@garious
Copy link
Contributor

garious commented Jul 16, 2020

Adding context::current() to every RPC call isn't too fun. What if we added an option to tarpc::service such that each Context parameter was tucked under the hood?

@garious garious changed the title Adding a context is tedious Creating a context for each call is tedious Jul 16, 2020
@tikue
Copy link
Collaborator

tikue commented Jul 16, 2020

Thanks for the suggestion! I'm sympathetic to this point of view, but the problem is that the best default for a context would do things that context::current() doesn't do:

  • a context's trace context should inherit the caller's trace ID and have the caller's span as the parent span.
  • the deadline should be the caller's deadline minus now().

I would love for this to be the behavior of context::current(). If it were, I'd have fewer qualms about having it done implicitly. The problem is I am not aware of a good request-local library. Thread locals are insufficient because a request handler can spawn multiple futures that run on different threads (or indeed spawn regular threads in serving a request).

Other options:

  • You could also alias context::current, e.g. use tarpc::context::current as cc;
  • Could you have an extension trait impl'd by the client that adds rpc methods that set up a context implicitly?

@garious
Copy link
Contributor Author

garious commented Jul 16, 2020

The extension trait (with #[async_trait]) is what I'm doing now. It works, but just wanted to offer the feedback, because in hindsight, I think I'd rather not give the user that level of control initially. If they needed it, I'd want to add it incrementally (pay for what you use).

The decreasing deadline doesn't feel like a critical invariant. Instead, I'd expect the timeout would generally need to increase with the number of network hops.

@drewkett
Copy link

drewkett commented Feb 13, 2021

It seems like the concerns are related to the situation where a server makes rpc calls to another server. In the scenario where that's not the case (which is how I use it) using context::current for everything seems fine. To handle this scenario, could the service macro generate a second client struct implementation like HelloClientDefaultContext (there's probably a better name) that could be used if you're fine with a default context.

Edit: Maybe SimpleHelloClient is a better name

@tikue
Copy link
Collaborator

tikue commented Jun 7, 2022

@drewkett apologies for never responding to your suggestion! I could potentially see value in an opt-in simple client — I wouldn't want it to be generated by default since it's more codegen that some users won't need.

One alternative would be an alias for context::current, like:

use tarpc::context::current as c;

then you could write something like client.hello(c(), "drewkett")?

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

No branches or pull requests

3 participants