tarpc icon indicating copy to clipboard operation
tarpc copied to clipboard

Creating a context for each call is tedious

Open garious opened this issue 4 years ago • 4 comments

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 avatar Jul 16 '20 13:07 garious

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?

tikue avatar Jul 16 '20 17:07 tikue

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.

garious avatar Jul 16 '20 17:07 garious

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

drewkett avatar Feb 13 '21 15:02 drewkett

@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")?

tikue avatar Jun 07 '22 08:06 tikue