Connection Timeout for client request

[akiradeveloper]

Feature Request

Crates

• tonic
  • Add: https://github.com/hjr3/hyper-timeout: This provides TimeoutConnector. I think this is doing basically the same thing as connect_timeout in reqwest because both uses tokio::time::Timeout wrapper.

Motivation

We can send a http/https request using Endpoint but it doesn't support connection timeout.

Yes, it has timeout method but it is a timeout for the entire request.

Proposal

Add connect_timeout to Endpoint and wrap the connector by TimeoutConnector.

    pub async fn connect(&self) -> Result<Channel, Error> {
        let mut http = hyper::client::connect::HttpConnector::new();
        http.enforce_http(false);
        http.set_nodelay(self.tcp_nodelay);
        http.set_keepalive(self.tcp_keepalive);

        // like this:
        // let http = TimeoutConnector::new(http);
        // http.set_connect_timeout(self.connect_timeout);
        //
        // see: https://github.com/hjr3/hyper-timeout/blob/0.3.1/examples/client.rs

        #[cfg(feature = "tls")]
        let connector = service::connector(http, self.tls.clone());

        #[cfg(not(feature = "tls"))]
        let connector = service::connector(http);

        Channel::connect(connector, self.clone()).await
    }

Alternatives

(1)

TimeoutConnector is just a Service -> Service function. So other solution is add stacking_fn: Service -> Service to connect function. This is a more versatile solution.

(2)

Endpoint has 3 connect variants but they all call Channel::connect at the end. My idea is factoring out the code before that. This make it easy to wrap the connector by users.

    pub async fn make_connector(&self) -> impl Service<Uri> {
        let mut http = hyper::client::connect::HttpConnector::new();
        http.enforce_http(false);
        http.set_nodelay(self.tcp_nodelay);
        http.set_keepalive(self.tcp_keepalive);

        #[cfg(feature = "tls")]
        let connector = service::connector(http, self.tls.clone());

        #[cfg(not(feature = "tls"))]
        let connector = service::connector(http);

        connector
    }

[akiradeveloper]

Here is a PoC code that insert TimeoutConnector before passing connector to Channel.

$ git di
diff --git a/tonic/Cargo.toml b/tonic/Cargo.toml
index 39b0276..f355d89 100644
--- a/tonic/Cargo.toml
+++ b/tonic/Cargo.toml
@@ -65,6 +65,7 @@ async-trait = { version = "0.1.13", optional = true }

 # transport
 hyper = { version = "0.13.4", features = ["stream"], optional = true }
+hyper-timeout = "0.3.1"
 tokio = { version = "0.2.13", features = ["tcp"], optional = true }
 tower = { version = "0.3", optional = true}
 tower-make = { version = "0.3", features = ["connect"] }
diff --git a/tonic/src/transport/channel/endpoint.rs b/tonic/src/transport/channel/endpoint.rs
index 788fa2e..6a4a744 100644
--- a/tonic/src/transport/channel/endpoint.rs
+++ b/tonic/src/transport/channel/endpoint.rs
@@ -243,6 +243,8 @@ impl Endpoint {
         #[cfg(not(feature = "tls"))]
         let connector = service::connector(http);

+        let mut connector = hyper_timeout::TimeoutConnector::new(connector);
+
         Channel::connect(connector, self.clone()).await
     }

@@ -262,6 +264,8 @@ impl Endpoint {
         #[cfg(not(feature = "tls"))]
         let connector = service::connector(http);

+        let mut connector = hyper_timeout::TimeoutConnector::new(connector);
+
         Ok(Channel::new(connector, self.clone()))
     }

@@ -283,6 +287,8 @@ impl Endpoint {
         #[cfg(not(feature = "tls"))]
         let connector = service::connector(connector);

+        let mut connector = hyper_timeout::TimeoutConnector::new(connector);
+
         Channel::connect(connector, self.clone()).await
     }

[LucioFranco]

This seems like a good idea 👍 I plan on doing some rework of the transport so will keep this in mind. Sorry for the delay.

[fujiapple852]

@akiradeveloper is there an advantage in using the hyper-timeout crate over using Hyper's set_connect_timeout method?

I recently hit an issue with connection timeouts in Tonic and resolved it with this PR.

Note that you can also work around this by using connect_with_connector and supplying a Hyper HttpConnector with the connect timeout set appropriately, though this does expose the Tonic client to Hyper directly which is not ideal.

@LucioFranco depending on when you intend to do the rework of the transport code, would you consider an interim PR such as any of the options discussed above to add the ability to set a connect timeout?

[akiradeveloper]

@fujiapple852

is there an advantage

No I don't think so. There can be possible implementations but your way looks better to me. I am looking forward to the new feature.