Option to keep stream connections open if even no Keepalive option is received.

src/net/client/stream.rs

@@ -20,6 +20,7 @@ use crate::base::opt::{AllOptData, OptRecord, TcpKeepalive};
 use crate::net::client::request::{
     ComposeRequest, Error, GetResponse, SendRequest,
 };
+use crate::utils::config::DefMinMax;
 use bytes::{Bytes, BytesMut};
 use core::cmp;
 use octseq::Octets;
@@ -39,13 +40,31 @@ use tokio::time::sleep;
 /// Default response timeout.
 ///
 /// Note: nsd has 120 seconds, unbound has 3 seconds.
-const DEF_RESPONSE_TIMEOUT: Duration = Duration::from_secs(19);
+const RESPONSE_TIMEOUT: DefMinMax<Duration> = DefMinMax::new(
+    Duration::from_secs(19),
+    Duration::from_millis(1),
+    Duration::from_secs(600),
+);
 
-/// Minimum configuration value for the response timeout.
-const MIN_RESPONSE_TIMEOUT: Duration = Duration::from_millis(1);
-
-/// Maximum configuration value for the response timeout.
-const MAX_RESPONSE_TIMEOUT: Duration = Duration::from_secs(600);
+/// Default idle timeout.
+///
+/// Note that RFC 7766, Secton 6.2.3 says: "DNS clients SHOULD close the
+/// TCP connection of an idle session, unless an idle timeout has been
+/// established using some other signalling mechanism, for example,
+/// [edns-tcp-keepalive]."
+/// However, RFC 7858, Section 3.4 says: "In order to amortize TCP and TLS
+/// connection setup costs, clients and servers SHOULD NOT immediately close
+/// a connection after each response.  Instead, clients and servers SHOULD
+/// reuse existing connections for subsequent queries as long as they have
+/// sufficient resources.".
+/// We set the default to 10 seconds, which is that same as what stubby
+/// uses. Minimum zero to allow idle timeout to be disabled. Assume that
+/// one hour is more than enough as maximum.
+const IDLE_TIMEOUT: DefMinMax<Duration> = DefMinMax::new(
+    Duration::from_secs(10),
+    Duration::ZERO,
+    Duration::from_secs(3600),
+);
 
 /// Capacity of the channel that transports `ChanReq`s.
 const DEF_CHAN_CAP: usize = 8;
@@ -60,6 +79,12 @@ const READ_REPLY_CHAN_CAP: usize = 8;
 pub struct Config {
     /// Response timeout.
     response_timeout: Duration,
+
+    /// Default idle timeout.
+    ///
+    /// This value is used if the other side does not send a TcpKeepalive
+    /// option.
+    idle_timeout: Duration,
 }
 
 impl Config {
@@ -77,22 +102,21 @@ impl Config {
     }
 
     /// Sets the response timeout.
-    ///
-    /// Excessive values are quietly trimmed.
-    //
-    //  XXX Maybe that’s wrong and we should rather return an error?
     pub fn set_response_timeout(&mut self, timeout: Duration) {
-        self.response_timeout = cmp::max(
-            cmp::min(timeout, MAX_RESPONSE_TIMEOUT),
-            MIN_RESPONSE_TIMEOUT,
-        )
+        self.response_timeout = RESPONSE_TIMEOUT.limit(timeout);
+    }
+
+    /// Sets the idle timeout.
+    pub fn set_idle_timeout(&mut self, timeout: Duration) {
+        self.idle_timeout = IDLE_TIMEOUT.limit(timeout)
     }
 }
 
 impl Default for Config {
     fn default() -> Self {
         Self {
-            response_timeout: DEF_RESPONSE_TIMEOUT,
+            response_timeout: RESPONSE_TIMEOUT.default(),
+            idle_timeout: IDLE_TIMEOUT.default(),
         }
     }
 }
@@ -265,9 +289,9 @@ struct Status {
 
     /// Time we are allow to keep the connection open when idle.
     ///
-    /// Initially we assume that the idle timeout is zero. A received
+    /// Initially we set the idle timeout to the default in config. A received
     /// edns-tcp-keepalive option may change that.
-    idle_timeout: Option<Duration>,
+    idle_timeout: Duration,
 }
 
 /// Status of the connection. Used in [`Status`].
@@ -334,7 +358,7 @@ where
 
         let mut status = Status {
             state: ConnState::Active(None),
-            idle_timeout: None,
+            idle_timeout: self.config.idle_timeout,
             send_keepalive: true,
         };
         let mut query_vec = Queries::new();
@@ -361,18 +385,14 @@ where
                     }
                 }
                 ConnState::Idle(instant) => {
-                    if let Some(timeout) = &status.idle_timeout {
-                        let elapsed = instant.elapsed();
-                        if elapsed >= *timeout {
-                            // Move to IdleTimeout and end
-                            // the loop
-                            status.state = ConnState::IdleTimeout;
-                            break;
-                        }
-                        Some(*timeout - elapsed)
-                    } else {
-                        panic!("Idle state but no timeout");
+                    let elapsed = instant.elapsed();
+                    if elapsed >= status.idle_timeout {
+                        // Move to IdleTimeout and end
+                        // the loop
+                        status.state = ConnState::IdleTimeout;
+                        break;
                     }
+                    Some(status.idle_timeout - elapsed)
                 }
                 ConnState::IdleTimeout
                 | ConnState::ReadError(_)
@@ -623,7 +643,7 @@ where
             // this independent.
             status.state = ConnState::Active(None);
 
-            status.state = if status.idle_timeout.is_none() {
+            status.state = if status.idle_timeout.is_zero() {
                 // Assume that we can just move to IdleTimeout
                 // state
                 ConnState::IdleTimeout
@@ -723,7 +743,7 @@ where
     fn handle_keepalive(opt_value: TcpKeepalive, status: &mut Status) {
         if let Some(value) = opt_value.timeout() {
             let value_dur = Duration::from(value);
-            status.idle_timeout = Some(value_dur);
+            status.idle_timeout = value_dur;
         }
     }