Make Endpoint connectors swappable
#148
Conversation
|
@akshayknarayan cargo-deny check is failing in part because hyper-unix-connector depends in hyperlocal, which in turn depends on hyper 0.12. Tonic depends on hyper 0.13.0-alpha.4 and cargo check is banning the duplicate dependency. |
tonic-build/src/client.rs
Outdated
| /// connector. | ||
| pub async fn connect_with_connector<C, D>(dst: D, connector: C) -> Result<Self, tonic::transport::Error> | ||
| where | ||
| C: tonic::transport::MakeConnection<http::Uri> + Send + 'static, |
There was a problem hiding this comment.
can we clean up some of these imports these seems a bit verbose 😄
tonic/src/transport/endpoint.rs
Outdated
| /// let endpoint = Endpoint::from_static("http://example.com"); | ||
| /// endpoint.connect_with_connector(UnixClient); //.await | ||
| /// ``` | ||
| pub async fn connect_with_connector<C>(&self, connector: C) -> Result<Channel, super::Error> |
There was a problem hiding this comment.
so I think it makes more sense for the endpoint to contain a connector option that by default uses the built in tonic one. So instead of providing it at connect time its provided with the endpoint. This means in a load balance list of endpoints they can each have their own type of connector. This should also simplify a lot of the code too.
There was a problem hiding this comment.
I have poked around at this idea a bit, and it probably involves making several things pub, like transport::service::Connector, which in turn would require exposing transport::service::io::BoxedIo, etc.
This is because having Endpoint<C: MakeConnection<...>> means we will have
pub fn tls_config(self, tls_config: ClientTlsConfig) -> Endpoint<transport::service::Connector> {and transport::service::Connector is currently pub(crate).
What is your plan regarding the tls feature? Things might be simpler if it was in its own crate - then usage would just be
Endpoint::from(...).connector(/* tls connector from the external crate */).connect().awaitwith a plain HTTPConnector as the default.
There was a problem hiding this comment.
You can use a Box<dyn Connector> type to avoid the generic for now. This should allow users to include it or not without generics.
|
It looks like we have the same issue here as in #151 with |
Change the `connect()` API to lift up the use of `connector()` to `Endpoint::connect()`, allowing users to provide their own implementations (for example, Unix-domain sockets). Any type which impls `tower_make::MakeConnection` is suitable. To avoid breaking the default case of HTTP(S), introduce `connect_with_connector()` and retain `connect()`, which creates the default connector according to the activated feature gate and passes it to `connect_with_connector()`. Fixes: hyperium#136
Instead of providing a connector in the call to `connect()`, make `Endpoint` contain a connector, by default an `HTTPConnector`, which the caller can swap.
akshayknarayan
force-pushed
the
connector
branch
from
2969071
to
fd74709
Compare
|
Hey! I opened #184 which adds this and a UDS example. Thank you for contributing this, I am gonna close this PR <3! Let me know if the other one doesn't meet your needs! |
Motivation
As discussed in #136, it would be useful if tonic supported other transports other than TCP.
The Server side is relatively straightforward to do, like so:
The client side is what this PR addresses.
Solution
Change the
connect()API to lift up the use ofconnector()toEndpoint::connect(), allowing users to provide their own implementations (for example, Unix-domain sockets). Any type which implstower_make::MakeConnectionis suitable.To avoid breaking the default case of HTTP(S), introduce
connect_with_connector()and retainconnect(), which creates the default connector according to the activated feature gate and passes it toconnect_with_connector().