Allow futures on Serve and Stub to be Send #448
Conversation
|
Sorry for the delay in reviewing! Will try to look in the next week. |
| @@ -66,6 +68,27 @@ impl Config { | |||
| } | |||
| } | |||
|
|
|||
| /// A [`Stub`] implementation that simply warps a `Serve`. | |||
There was a problem hiding this comment.
Nit: typo
| /// A [`Stub`] implementation that simply warps a `Serve`. | |
| /// A [`Stub`] implementation that simply wraps a `Serve`. |
| @@ -66,6 +68,27 @@ impl Config { | |||
| } | |||
| } | |||
|
|
|||
| /// A [`Stub`] implementation that simply warps a `Serve`. | |||
| pub struct ServeStub<S> { | |||
There was a problem hiding this comment.
Maybe keep this type private for now and return an imp Stub? Can always make it public later if needed.
Perfect, that's what I was looking for by putting up an early draft.
Probably, was just focusing on my use case a little too much 😅
Will do, as I mentioned this is an early draft to make sure the change makes sense. |
|
I'll get back to this in a day or so, thanks for the review! |
ShaneMurphy2
force-pushed
the
trait-variant
branch
from
304be66
to
7f6f351
Compare
|
Haven't forgotten, just wrapped up in other stuff. I'll try to get to it soon! |
|
Looks like Also, how do you feel about renaming Serve and Stub to LocalServe and LocalStub so that the "default" type is |
|
I've given it some more thought and I'm wondering if the right move is to instead have a feature flag that annotates all traits with async methods in them with |
|
I also just saw rust-lang/rfcs#3654. Wonder how long it will take to be available on nightly? |
Looks like even post-implementation the RFC still recommends the pattern provided by |
|
Sorry for ping, any updates? |
I'm currently using my fork's branch |
There was a problem hiding this comment.
Hey @ShaneMurphy2 taking another look at this in a while. Are you still interested in getting this merged?
Also, I'm kind of curious about trying out a feature-gated implementation that uses return-type notation rather than trait_variant. Some related thoughts on #480
tarpc/src/client/stub.rs
Outdated
| @@ -16,6 +14,7 @@ mod mock; | |||
| /// A connection to a remote service. | |||
| /// Calls the service with requests of type `Req` and receives responses of type `Resp`. | |||
| #[allow(async_fn_in_trait)] | |||
| #[trait_variant::make(TokioStub: Send)] | |||
There was a problem hiding this comment.
Nit: maybe just call it SendStub?
tarpc/Cargo.toml
Outdated
| @@ -49,7 +49,7 @@ pin-project = "1.0" | |||
| rand = "0.8" | |||
| serde = { optional = true, version = "1.0", features = ["derive"] } | |||
| static_assertions = "1.1.0" | |||
| tarpc-plugins = { path = "../plugins", version = "0.13" } | |||
| tarpc-plugins = { path = "../plugins", version = "0.13", registry = "umbra"} | |||
There was a problem hiding this comment.
Is the registry = "umbra" bit supposed to be here?
There was a problem hiding this comment.
Nope, coworker pushed to the wrong branch.
| @@ -77,6 +100,16 @@ pub trait Serve { | |||
|
|
|||
| /// Responds to a single request. | |||
| async fn serve(self, ctx: context::Context, req: Self::Req) -> Result<Self::Resp, ServerError>; | |||
|
|
|||
| /// Wrap this `Serve` in a type that implements [`Stub`]. | |||
| async fn into_stub(self) -> ServeStub<Self> | |||
There was a problem hiding this comment.
Can you remind me why the blanket impl for Serve impls had to be removed?
There was a problem hiding this comment.
I think the compiler was complaining about conflicting trait implementations.
ShaneMurphy2
force-pushed
the
trait-variant
branch
from
b8ddc3a
to
a272ab8
Compare
I'm ambivalent on getting it merged as I think we can do better with the API, and I'm not confident this is the right approach in the general case. It works for our use case and a fork is ok for us for now.
That could be neat, I agree with your sentiment about being "forward thinking". In a similar vein with using feature-gating, it could be nice to put the |
Not sure if this approach is the right one, so this MR is a draft.