Remove connect timeouts from Bind (#487)

Currently, the `Reconnect` middleware does not reconnect on connection errors (see  #491) and treats them as request errors. This means that when a connection timeout is wrapped in a `Reconnect`, timeout errors are treated as request errors, and the request returns HTTP 500. Since  this is not the desired behavior, the connection timeouts should be removed, at least until their errors can be handled differently.

This PR removes the connect timeouts from `Bind`, as described in https://github.com/runconduit/conduit/pull/483#issuecomment-369380003.

It removes the `CONDUIT_PROXY_PUBLIC_CONNECT_TIMEOUT_MS` environment variable, but _not_ the `CONDUIT_PROXY_PRIVATE_CONNECT_TIMEOUT_MS` variable, since this is also used for the TCP connect timeouts. If we want also want to remove the TCP connection timeouts, I can do that as well.

Closes #483. Fixes #491.

proxy/src/bind.rs

@@ -4,7 +4,6 @@ use std::marker::PhantomData;
 use std::net::SocketAddr;
 use std::sync::Arc;
 use std::sync::atomic::AtomicUsize;
-use std::time::Duration;
 
 use http;
 use tokio_core::reactor::Handle;
@@ -18,9 +17,6 @@ use ctx;
 use telemetry::{self, sensor};
 use transparency::{self, HttpBody};
 use transport;
-use ::timeout::Timeout;
-
-const DEFAULT_TIMEOUT_MS: u64 = 300;
 
 /// Binds a `Service` from a `SocketAddr`.
 ///
@@ -34,7 +30,6 @@ pub struct Bind<C, B> {
     sensors: telemetry::Sensors,
     executor: Handle,
     req_ids: Arc<AtomicUsize>,
-    connect_timeout: Duration,
     _p: PhantomData<B>,
 }
 
@@ -58,7 +53,7 @@ pub type NewHttp<B> = sensor::NewHttp<Client<B>, B, HttpBody>;
 pub type HttpResponse = http::Response<sensor::http::ResponseBody<HttpBody>>;
 
 pub type Client<B> = transparency::Client<
-    sensor::Connect<transport::TimeoutConnect<transport::Connect>>,
+    sensor::Connect<transport::Connect>,
     B,
 >;
 
@@ -96,18 +91,10 @@ impl<B> Bind<(), B> {
             ctx: (),
             sensors: telemetry::Sensors::null(),
             req_ids: Default::default(),
-            connect_timeout: Duration::from_millis(DEFAULT_TIMEOUT_MS),
             _p: PhantomData,
         }
     }
 
-    pub fn with_connect_timeout(self, connect_timeout: Duration) -> Self {
-        Self {
-            connect_timeout,
-            ..self
-        }
-    }
-
     pub fn with_sensors(self, sensors: telemetry::Sensors) -> Self {
         Self {
             sensors,
@@ -121,7 +108,6 @@ impl<B> Bind<(), B> {
             sensors: self.sensors,
             executor: self.executor,
             req_ids: self.req_ids,
-            connect_timeout: self.connect_timeout,
             _p: PhantomData,
         }
     }
@@ -134,7 +120,6 @@ impl<C: Clone, B> Clone for Bind<C, B> {
             sensors: self.sensors.clone(),
             executor: self.executor.clone(),
             req_ids: self.req_ids.clone(),
-            connect_timeout: self.connect_timeout,
             _p: PhantomData,
         }
     }
@@ -142,9 +127,6 @@ impl<C: Clone, B> Clone for Bind<C, B> {
 
 
 impl<C, B> Bind<C, B> {
-    pub fn connect_timeout(&self) -> Duration {
-        self.connect_timeout
-    }
 
     // pub fn ctx(&self) -> &C {
     //     &self.ctx
@@ -177,15 +159,10 @@ where
         );
 
         // Map a socket address to a connection.
-        let connect = {
-            let c = Timeout::new(
-                transport::Connect::new(*addr, &self.executor),
-                self.connect_timeout,
-                &self.executor,
-            );
-
-            self.sensors.connect(c, &client_ctx)
-        };
+        let connect = self.sensors.connect(
+            transport::Connect::new(*addr, &self.executor),
+            &client_ctx
+        );
 
         let client = transparency::Client::new(
             protocol,

proxy/src/config.rs

@@ -30,7 +30,7 @@ pub struct Config {
     pub private_forward: Option<Addr>,
 
     /// The maximum amount of time to wait for a connection to the public peer.
-    pub public_connect_timeout: Option<Duration>,
+    pub public_connect_timeout: Duration,
 
     /// The maximum amount of time to wait for a connection to the private peer.
     pub private_connect_timeout: Duration,
@@ -159,6 +159,7 @@ const DEFAULT_PRIVATE_LISTENER: &str = "tcp://127.0.0.1:4140";
 const DEFAULT_PUBLIC_LISTENER: &str = "tcp://0.0.0.0:4143";
 const DEFAULT_CONTROL_LISTENER: &str = "tcp://0.0.0.0:4190";
 const DEFAULT_PRIVATE_CONNECT_TIMEOUT_MS: u64 = 20;
+const DEFAULT_PUBLIC_CONNECT_TIMEOUT_MS: u64 = 300;
 const DEFAULT_BIND_TIMEOUT_MS: u64 = 10_000; // ten seconds, as in Linkerd.
 const DEFAULT_RESOLV_CONF: &str = "/etc/resolv.conf";
 
@@ -215,7 +216,10 @@ impl<'a> TryFrom<&'a Strings> for Config {
                     .unwrap_or_else(|| Addr::from_str(DEFAULT_CONTROL_LISTENER).unwrap()),
             },
             private_forward: private_forward?,
-            public_connect_timeout: public_connect_timeout?.map(Duration::from_millis),
+            public_connect_timeout: Duration::from_millis(
+                public_connect_timeout?
+                    .unwrap_or(DEFAULT_PUBLIC_CONNECT_TIMEOUT_MS)
+            ),
             private_connect_timeout:
                 Duration::from_millis(private_connect_timeout?
                                           .unwrap_or(DEFAULT_PRIVATE_CONNECT_TIMEOUT_MS)),

proxy/src/lib.rs

@@ -188,9 +188,7 @@ where
         let inbound = {
             let ctx = ctx::Proxy::inbound(&process_ctx);
 
-            let bind = bind.clone()
-                .with_connect_timeout(config.private_connect_timeout)
-                .with_ctx(ctx.clone());
+            let bind = bind.clone().with_ctx(ctx.clone());
 
             let default_addr = config.private_forward.map(|a| a.into());
 
@@ -212,12 +210,7 @@ where
         let outbound = {
             let ctx = ctx::Proxy::outbound(&process_ctx);
 
-            let bind = config
-                .public_connect_timeout
-                .map_or_else(|| bind.clone(), |t| bind.clone().with_connect_timeout(t))
-                .with_ctx(ctx.clone());
-
-            let tcp_connect_timeout = bind.connect_timeout();
+            let bind = bind.clone().with_ctx(ctx.clone());
 
             let outgoing = Outbound::new(
                 bind,
@@ -230,7 +223,7 @@ where
             let fut = serve(
                 outbound_listener,
                 outgoing,
-                tcp_connect_timeout,
+                config.public_connect_timeout,
                 ctx,
                 sensors,
                 get_original_dst,

proxy/src/transport/connect.rs

@@ -11,7 +11,6 @@ use http;
 use connection;
 use convert;
 use dns;
-use ::timeout;
 
 #[derive(Debug, Clone)]
 pub struct Connect {
@@ -44,8 +43,6 @@ pub struct LookupAddressAndConnect {
     handle: Handle,
 }
 
-pub type TimeoutConnect<C> = timeout::Timeout<C>;
-
 // ===== impl HostAndPort =====
 
 impl<'a> convert::TryFrom<&'a http::uri::Authority> for HostAndPort {

proxy/src/transport/mod.rs

@@ -5,6 +5,5 @@ pub use self::connect::{
     Connect,
     Host, HostAndPort, HostAndPortError,
     LookupAddressAndConnect,
-    TimeoutConnect
 };
 pub use self::so_original_dst::{GetOriginalDst, SoOriginalDst};