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

Map transports not services #106

Merged
merged 33 commits into from
Nov 6, 2024
Merged

Map transports not services #106

merged 33 commits into from
Nov 6, 2024

Conversation

rklaehn
Copy link
Collaborator

@rklaehn rklaehn commented Nov 2, 2024

TLDR: instead of mapping the service, map the transport. This gets rid of the mapper in the service and makes the RpcClient/RpcChannel and friends again just pure newtype like things.

To make this work I had to make a bigger change. The various channel traits now have associated types In and Out, instead of type parameters.

  • The three service types (RpcClient, RpcServer and RpcChannel) are now again pure newtypes over the underlying transports that just add the service type.

  • map is available on RpcClient and RpcChannel, but unlike before it maps the underlying transport, so there is no third type parameter SC needed anymore

  • the boxed transport is available by default, .boxed() is available on all transports including those that are already boxed. For RpcClient, RpcChannel and RpcServer, the transport type C is set by default to the boxed transport. That way in many cases you can avoid having to provide the transport type parameter at all, at the expense of a tiny bit of performance.

  • also made the combined transport available by default - it does not have any dependencies, so it seems like having a feature flag for it is overkill.

@rklaehn rklaehn marked this pull request as ready for review November 3, 2024 20:52
They don't bring in deps, so why not. Boxed is universally useful,
and if it is not default I can't use it as default.
@@ -53,23 +53,15 @@ impl<SC, S, C: Clone> Clone for RpcClient<S, C, SC> {
/// that support it, [crate::message::ClientStreaming] and [crate::message::BidiStreaming].
#[pin_project]
#[derive(Debug)]
pub struct UpdateSink<SC, C, T, S = SC>(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Weird that the phantomdata is pub -> fixed

also make sure boxing is more convenient
@dignifiedquire
Copy link
Contributor

there is a lot of changes, is there a specific file showing the benefits of this change, to understand how it helps the user?

@rklaehn
Copy link
Collaborator Author

rklaehn commented Nov 4, 2024

there is a lot of changes, is there a specific file showing the benefits of this change, to understand how it helps the user?

The closest thing to that is the smoke "test" in mapped.rs

As for a real use case: when extracting the rpc for blobs, I had the following problem. Either you have to drag around the service type parameter matching the transport channel SC everywhere, or you have to define a type alias. With this PR you simply do not have the SC type parameter anymore, so no need to drag it around.

E.g. you got blobs::Client::new. You can either make it take a RpcClient, having no type parameters at all. Then you create one by saying blobs::Client::new(iroh_rpc_client.map().boxed()).

...or take a RpcClient<BlobsService, C> where C is a channel that is compatible with BlobsService and defaults to BoxedServiceConnection. Then you can call it with blobs::Client::new(iroh_rpc_client.map()) and don't have the (small) boxing overhead.

In any case, you avoid having to have an open S: Service parameter at all in the iroh blobs client and service implementation, which at least while writing it I found horribly confusing...

@rklaehn rklaehn merged commit b0341a1 into main Nov 6, 2024
14 of 15 checks passed
@rklaehn rklaehn deleted the map-transports-not-services branch November 6, 2024 08:46
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.

2 participants