Repository: seanmonstar/reqwest (language: rust)

Issue:
TCP keep alives not enabled when using reqwest causing it to block forever
Client sockets in reqwest do not have tcp keep alives enabled, you can verify this with the `ss` command.

```
$ sudo ss -iepn | grep tcp
tcp    ESTAB      0      0      192.168.180.3:34252              34.98.70.137:443                 users:(("home",pid=19690,fd=4)) ino:38024387 sk:5 <->
```
This causes the socket to be forever stuck in a tcp ESTAB state because the kernel will never notice the socket has been closed. This in turn caused my rust program that uses the blocking reqwest api to hang forever even when the server I was connecting to closed its socket days before.

Hyper has tcp keep alives enabled by default. Reqwest should as well.

---

I am new to rust, but I believe keep alives are not being set (`set_keepalive` is never called) because reqwest uses its own Connector.

```
        let mut builder = hyper::Client::builder(); # has the default connector, which would have tcp keep alives
        ... 
        let hyper_client = builder.build(connector); # uses reqwest's custom connector, with no tcp keep alives
```

Here is a small diff which will correctly set tcp keep alives in reqwest:

```
diff --git a/src/async_impl/client.rs b/src/async_impl/client.rs
index 1faa3b4..6cae2d4 100644
--- a/src/async_impl/client.rs
+++ b/src/async_impl/client.rs
@@ -310,6 +310,7 @@ impl ClientBuilder {
 
         builder.pool_idle_timeout(config.pool_idle_timeout);
         builder.pool_max_idle_per_host(config.pool_max_idle_per_host);
+        connector.set_keepalive(config.pool_idle_timeout);
 
         if config.http1_title_case_headers {
             builder.http1_title_case_headers(true);
diff --git a/src/connect.rs b/src/connect.rs
index f52a978..b782b69 100644
--- a/src/connect.rs
+++ b/src/connect.rs
@@ -71,6 +71,7 @@ impl_http_connector! {
     fn set_local_address(&mut self, addr: Option<IpAddr>);
     fn enforce_http(&mut self, is_enforced: bool);
     fn set_nodelay(&mut self, nodelay: bool);
+    fn set_keepalive(&mut self, dur: Option<Duration>);
 }
 
 impl Service<Uri> for HttpConnector {
@@ -315,11 +316,18 @@ impl Connector {
         })
     }
 
-    async fn connect_with_maybe_proxy(
-        self,
-        dst: Uri,
-        is_proxy: bool,
-    ) -> Result<Conn, BoxError> {
+    pub fn set_keepalive(&mut self, dur: Option<Duration>) {
+        match &mut self.inner {
+            #[cfg(feature = "default-tls")]
+            Inner::DefaultTls(http, _tls) => http.set_keepalive(dur),
+            #[cfg(feature = "rustls-tls")]
+            Inner::RustlsTls { http, .. } => http.set_keepalive(dur),
+            #[cfg(not(feature = "__tls"))]
+            Inner::Http(http) => http.set_keepalive(dur),
+        }
+    }
+
+    async fn connect_with_maybe_proxy(self, dst: Uri, is_proxy: bool) -> Result<Conn, BoxError> {
         match self.inner {
             #[cfg(not(feature = "__tls"))]
             Inner::Http(mut http) => {
-- 
2.25.1
```

I was able to verify that keep alives were correctly enabled after making the changes:

```
$ sudo ss -iepn | grep home | grep tcp
tcp    ESTAB      0      0      192.168.180.3:34302              34.98.70.137:443                 users:(("home",pid=20743,fd=4)) timer:(keepalive,18sec,0) ino:38026649 sk:6 <->
```



Discussion / hints:
This is a great report, thanks! You're absolutely right, we forget to set TCP keepalive anywhere.

I know hyper currently sets the value based on the pool idle timeout, but I'm no longer sure it's correct to relate those two things together. Perhaps the `ClientBuilder` in request should grow `tcp_keepalive(impl Into<Option<Duration>>)`, and we can use a sensible default. Passing `None` would disable the default. What do you think?

Fix patch:
diff --git a/src/async_impl/client.rs b/src/async_impl/client.rs
index 75610e665..ff103f77b 100644
--- a/src/async_impl/client.rs
+++ b/src/async_impl/client.rs
@@ -80,6 +80,7 @@ struct Config {
     connection_verbose: bool,
     pool_idle_timeout: Option<Duration>,
     pool_max_idle_per_host: usize,
+    tcp_keepalive: Option<Duration>,
     #[cfg(feature = "__tls")]
     identity: Option<Identity>,
     proxies: Vec<Proxy>,
@@ -131,6 +132,7 @@ impl ClientBuilder {
                 connection_verbose: false,
                 pool_idle_timeout: Some(Duration::from_secs(90)),
                 pool_max_idle_per_host: std::usize::MAX,
+                tcp_keepalive: Some(Duration::from_secs(60)),
                 proxies: Vec::new(),
                 auto_sys_proxy: true,
                 redirect_policy: redirect::Policy::default(),
@@ -316,6 +318,7 @@ impl ClientBuilder {
 
         builder.pool_idle_timeout(config.pool_idle_timeout);
         builder.pool_max_idle_per_host(config.pool_max_idle_per_host);
+        connector.set_keepalive(config.tcp_keepalive);
 
         if config.http1_title_case_headers {
             builder.http1_title_case_headers(true);
@@ -712,6 +715,19 @@ impl ClientBuilder {
         self
     }
 
+    /// Set that all sockets have `SO_KEEPALIVE` set with the supplied duration.
+    ///
+    /// If `None`, the option will not be set.
+    ///
+    /// Default is 60 seconds.
+    pub fn tcp_keepalive<D>(mut self, val: D) -> ClientBuilder
+        where
+            D: Into<Option<Duration>>,
+    {
+        self.config.tcp_keepalive = val.into();
+        self
+    }
+
     // TLS options
 
     /// Add a custom root certificate.
diff --git a/src/connect.rs b/src/connect.rs
index f52a978ce..7d5202cf5 100644
--- a/src/connect.rs
+++ b/src/connect.rs
@@ -71,6 +71,7 @@ impl_http_connector! {
     fn set_local_address(&mut self, addr: Option<IpAddr>);
     fn enforce_http(&mut self, is_enforced: bool);
     fn set_nodelay(&mut self, nodelay: bool);
+    fn set_keepalive(&mut self, dur: Option<Duration>);
 }
 
 impl Service<Uri> for HttpConnector {
@@ -474,6 +475,17 @@ impl Connector {
 
         self.connect_with_maybe_proxy(proxy_dst, true).await
     }
+
+    pub fn set_keepalive(&mut self, dur: Option<Duration>) {
+        match &mut self.inner {
+            #[cfg(feature = "default-tls")]
+            Inner::DefaultTls(http, _tls) => http.set_keepalive(dur),
+            #[cfg(feature = "rustls-tls")]
+            Inner::RustlsTls { http, .. } => http.set_keepalive(dur),
+            #[cfg(not(feature = "__tls"))]
+            Inner::Http(http) => http.set_keepalive(dur),
+        }
+    }
 }
 
 fn into_uri(scheme: Scheme, host: Authority) -> Uri {


Test patch: