From ad073c79b9bd4cc8e5c4396a1c4248ae28c1da10 Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Mon, 5 Mar 2018 15:38:20 -0800 Subject: [PATCH] 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 | 33 +++++---------------------------- proxy/src/config.rs | 8 ++++++-- proxy/src/lib.rs | 13 +++---------- proxy/src/transport/connect.rs | 3 --- proxy/src/transport/mod.rs | 1 - 5 files changed, 14 insertions(+), 44 deletions(-) diff --git a/proxy/src/bind.rs b/proxy/src/bind.rs index e0bc2e940..0233089c6 100644 --- a/proxy/src/bind.rs +++ b/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 { sensors: telemetry::Sensors, executor: Handle, req_ids: Arc, - connect_timeout: Duration, _p: PhantomData, } @@ -58,7 +53,7 @@ pub type NewHttp = sensor::NewHttp, B, HttpBody>; pub type HttpResponse = http::Response>; pub type Client = transparency::Client< - sensor::Connect>, + sensor::Connect, B, >; @@ -96,18 +91,10 @@ impl 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 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 Clone for Bind { 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 Clone for Bind { impl Bind { - 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, diff --git a/proxy/src/config.rs b/proxy/src/config.rs index 304bcabd8..2457cb6c2 100644 --- a/proxy/src/config.rs +++ b/proxy/src/config.rs @@ -30,7 +30,7 @@ pub struct Config { pub private_forward: Option, /// The maximum amount of time to wait for a connection to the public peer. - pub public_connect_timeout: Option, + 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)), diff --git a/proxy/src/lib.rs b/proxy/src/lib.rs index 8efe4c376..03f67d6ab 100644 --- a/proxy/src/lib.rs +++ b/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, diff --git a/proxy/src/transport/connect.rs b/proxy/src/transport/connect.rs index a233bf3ab..bba7581c0 100644 --- a/proxy/src/transport/connect.rs +++ b/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 = timeout::Timeout; - // ===== impl HostAndPort ===== impl<'a> convert::TryFrom<&'a http::uri::Authority> for HostAndPort { diff --git a/proxy/src/transport/mod.rs b/proxy/src/transport/mod.rs index 6adf87e83..4440de7d5 100644 --- a/proxy/src/transport/mod.rs +++ b/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};