Allow futures on Serve and Stub to be Send by ShaneMurphy2 · Pull Request #448 · google/tarpc

ShaneMurphy2 · 2024-03-28T16:45:17Z

Not sure if this approach is the right one, so this MR is a draft.

tikue · 2024-04-12T03:26:41Z

Sorry for the delay in reviewing! Will try to look in the next week.

tikue

This looks like a reasonable change to me. Doesn't Serve need to be made a trait variant, too? Also, could you add some tests demonstrating how it will work for users?

tikue · 2024-04-24T05:39:54Z

tarpc/src/server.rs

    }
 }

+/// A [`Stub`] implementation that simply warps a `Serve`.


Nit: typo

Suggested change

/// A [`Stub`] implementation that simply warps a `Serve`.

/// A [`Stub`] implementation that simply wraps a `Serve`.

tikue · 2024-04-24T05:41:38Z

tarpc/src/server.rs

 }

+/// A [`Stub`] implementation that simply warps a `Serve`.
+pub struct ServeStub<S> {


Maybe keep this type private for now and return an imp Stub? Can always make it public later if needed.

ShaneMurphy2 · 2024-04-24T16:17:50Z

This looks like a reasonable change to me.

Perfect, that's what I was looking for by putting up an early draft.

Doesn't Serve need to be made a trait variant, too?

Probably, was just focusing on my use case a little too much 😅

Also, could you add some tests demonstrating how it will work for users?

Will do, as I mentioned this is an early draft to make sure the change makes sense.

ShaneMurphy2 · 2024-04-24T16:18:48Z

I'll get back to this in a day or so, thanks for the review!

ShaneMurphy2 · 2024-05-01T22:45:29Z

Haven't forgotten, just wrapped up in other stuff. I'll try to get to it soon!

ShaneMurphy2 · 2024-05-03T17:25:30Z

Looks like trait_variant::make doesn't support default implementations in trait definitions. I could update the macro in that crate, thoughts?

Also, how do you feel about renaming Serve and Stub to LocalServe and LocalStub so that the "default" type is Send, seems like the more common use case potentially.

ShaneMurphy2 · 2024-05-20T20:05:18Z

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 async_trait. Thoughts @tikue?

tikue · 2024-06-22T19:48:22Z

I also just saw rust-lang/rfcs#3654. Wonder how long it will take to be available on nightly?

ShaneMurphy2 · 2024-06-24T15:56:43Z

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 trait_variant: https://github.com/nikomatsakis/rfcs/blob/rtn/text/0000-return-type-notation.md#rtn-supports-convenient-trait-aliases
https://github.com/nikomatsakis/rfcs/blob/rtn/text/0000-return-type-notation.md#guidelines-and-best-practices

0xdeafbeef · 2024-07-31T14:28:55Z

Sorry for ping, any updates?

ShaneMurphy2 · 2024-09-04T19:55:41Z

Sorry for ping, any updates?

I'm currently using my fork's branch trait-variant and it seems to work fine. Probably that could be merged soon since it is backwards compatible with the old way, as it only adds Tokio* types that bound the futures to be Send.

tikue

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

tikue · 2024-10-27T00:35:41Z

tarpc/src/client/stub.rs

 /// 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)]


Nit: maybe just call it SendStub?

tikue · 2024-10-27T00:36:06Z

tarpc/Cargo.toml

 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"}


Is the registry = "umbra" bit supposed to be here?

Nope, coworker pushed to the wrong branch.

tikue · 2024-10-27T00:37:08Z

tarpc/src/server.rs

    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>


Can you remind me why the blanket impl for Serve impls had to be removed?

I think the compiler was complaining about conflicting trait implementations.

ShaneMurphy2 · 2024-10-27T04:49:09Z

Hey @ShaneMurphy2 taking another look at this in a while. Are you still interested in getting this merged?

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.

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

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 async_trait macro on all the traits behind a feature gate. That macro makes async traits very ergonomic for folks who can afford the performance hit.

ShaneMurphy2 added 2 commits March 15, 2024 11:03

Add trait variant

05c6cee

Replace blanket impl with Serve::to_stub

7f6f351

tikue marked this pull request as ready for review April 24, 2024 05:37

tikue requested changes Apr 24, 2024

View reviewed changes

ShaneMurphy2 force-pushed the trait-variant branch from 304be66 to 7f6f351 Compare April 25, 2024 19:45

tikue force-pushed the master branch from 2358986 to 6d56903 Compare June 29, 2024 13:55

stevefan1999-personal mentioned this pull request Oct 7, 2024

Let Serve and Stub traits be send #475

Closed

tikue reviewed Oct 27, 2024

View reviewed changes

Rename to SendStub

a272ab8

ShaneMurphy2 force-pushed the trait-variant branch from b8ddc3a to a272ab8 Compare October 27, 2024 04:44

tikue force-pushed the master branch from c28fa2e to 2b16620 Compare August 10, 2025 18:11

	/// A [`Stub`] implementation that simply warps a `Serve`.
	/// A [`Stub`] implementation that simply wraps a `Serve`.

Conversation

ShaneMurphy2 commented Mar 28, 2024

Uh oh!

tikue commented Apr 12, 2024

Uh oh!

tikue left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ShaneMurphy2 commented Apr 24, 2024

Uh oh!

ShaneMurphy2 commented Apr 24, 2024

Uh oh!

ShaneMurphy2 commented May 1, 2024

Uh oh!

ShaneMurphy2 commented May 3, 2024

Uh oh!

ShaneMurphy2 commented May 20, 2024

Uh oh!

tikue commented Jun 22, 2024

Uh oh!

ShaneMurphy2 commented Jun 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

0xdeafbeef commented Jul 31, 2024

Uh oh!

ShaneMurphy2 commented Sep 4, 2024

Uh oh!

tikue left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ShaneMurphy2 commented Oct 27, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ShaneMurphy2 commented Jun 24, 2024 •

edited

Loading