refactor!: Use Duration in config and centralise defaults

Rather than using names and documentation to specify the units for
durations, we can use the `Duration` type which lets callers choose
whatever works best for them. It also means we no longer have to convert
to `Duration` internally, simplifying some code.

Additionally, the default values for duration constants have been
centralised under `config`, which has been made public so users can see
them in documentation.

BREAKING CHANGE: The duration fields in config have all changed:

- `idle_timeout_msec` is renamed `idle_timeout` and is now an
  `Option<Duration>`.
- `keep_alive_interval_msec` is renamed `keep_alive_interval` and is now
  an `Option<Duration>`.
- `upnp_lease_duration` is now an `Option<Duration>`.
- `retry_duration_msec` is renamed `min_retry_duration` and is now an
  `Option<Duration>`.

examples/p2p_node.rs

@@ -15,8 +15,11 @@
 use anyhow::Result;
 use bytes::Bytes;
 use qp2p::{Config, ConnId, QuicP2p};
-use std::env;
-use std::net::{Ipv4Addr, SocketAddr};
+use std::{
+    env,
+    net::{Ipv4Addr, SocketAddr},
+    time::Duration,
+};
 
 #[derive(Default, Ord, PartialEq, PartialOrd, Eq, Clone, Copy)]
 struct XId(pub [u8; 32]);
@@ -37,7 +40,7 @@ async fn main() -> Result<()> {
 
     // instantiate QuicP2p with custom config
     let qp2p: QuicP2p<XId> = QuicP2p::with_config(Config {
-        idle_timeout_msec: Some(1000 * 3600), // 1 hour idle timeout.
+        idle_timeout: Duration::from_secs(60 * 60).into(), // 1 hour idle timeout.
         ..Default::default()
     })?;
 

src/api.rs

@@ -8,20 +8,20 @@
 // Software.
 
 use super::{
-    config::{Config, InternalConfig, SerialisableCertificate},
+    config::{
+        Config, InternalConfig, SerialisableCertificate, DEFAULT_IDLE_TIMEOUT,
+        DEFAULT_KEEP_ALIVE_INTERVAL, DEFAULT_MIN_RETRY_DURATION, DEFAULT_UPNP_LEASE_DURATION,
+    },
     connection_pool::ConnId,
     connections::DisconnectionEvents,
     endpoint::{Endpoint, IncomingConnections, IncomingMessages},
     error::{Error, Result},
-    peer_config::{self, DEFAULT_IDLE_TIMEOUT_MSEC, DEFAULT_KEEP_ALIVE_INTERVAL_MSEC},
+    peer_config,
 };
 use std::marker::PhantomData;
 use std::net::{SocketAddr, UdpSocket};
 use tracing::{debug, error, trace};
 
-/// Default duration of a UPnP lease, in seconds.
-pub(crate) const DEFAULT_UPNP_LEASE_DURATION_SEC: u32 = 120;
-
 const MAIDSAFE_DOMAIN: &str = "maidsafe.net";
 
 /// Main QuicP2p instance to communicate with QuicP2p using an async API
@@ -59,33 +59,31 @@ impl<I: ConnId> QuicP2p<I> {
     pub fn with_config(cfg: Config) -> Result<Self> {
         debug!("Config passed in to qp2p: {:?}", cfg);
 
-        let idle_timeout_msec = cfg.idle_timeout_msec.unwrap_or(DEFAULT_IDLE_TIMEOUT_MSEC);
-
-        let keep_alive_interval_msec = cfg
-            .keep_alive_interval_msec
-            .unwrap_or(DEFAULT_KEEP_ALIVE_INTERVAL_MSEC);
-
         let (key, cert) = {
             let our_complete_cert =
                 SerialisableCertificate::new(vec![MAIDSAFE_DOMAIN.to_string()])?;
             our_complete_cert.obtain_priv_key_and_cert()?
         };
 
-        let endpoint_cfg =
-            peer_config::new_our_cfg(idle_timeout_msec, keep_alive_interval_msec, cert, key)?;
-
-        let client_cfg = peer_config::new_client_cfg(idle_timeout_msec, keep_alive_interval_msec)?;
-
+        let idle_timeout = cfg.idle_timeout.unwrap_or(DEFAULT_IDLE_TIMEOUT);
+        let keep_alive_interval = cfg
+            .keep_alive_interval
+            .unwrap_or(DEFAULT_KEEP_ALIVE_INTERVAL);
         let upnp_lease_duration = cfg
             .upnp_lease_duration
-            .unwrap_or(DEFAULT_UPNP_LEASE_DURATION_SEC);
+            .unwrap_or(DEFAULT_UPNP_LEASE_DURATION);
+        let min_retry_duration = cfg.min_retry_duration.unwrap_or(DEFAULT_MIN_RETRY_DURATION);
+
+        let endpoint_cfg = peer_config::new_our_cfg(idle_timeout, keep_alive_interval, cert, key)?;
+
+        let client_cfg = peer_config::new_client_cfg(idle_timeout, keep_alive_interval)?;
 
         let qp2p_config = InternalConfig {
             forward_port: cfg.forward_port,
             external_port: cfg.external_port,
             external_ip: cfg.external_ip,
             upnp_lease_duration,
-            retry_duration_msec: cfg.retry_duration_msec,
+            min_retry_duration,
         };
 
         Ok(Self {

src/config.rs

@@ -7,52 +7,94 @@
 // specific language governing permissions and limitations relating to use of the SAFE Network
 // Software.
 
+//! Configuration for `Endpoint`s.
+
 use crate::{
     error::{Error, Result},
     utils,
 };
 use bytes::Bytes;
 use serde::{Deserialize, Serialize};
-use std::{fmt, net::IpAddr, str::FromStr};
+use std::{fmt, net::IpAddr, str::FromStr, time::Duration};
 use structopt::StructOpt;
 
+/// Default for [`Config::idle_timeout`] (1 minute).
+///
+/// This is based on average time in which routers would close the UDP mapping to the peer if they
+/// see no conversation between them.
+pub const DEFAULT_IDLE_TIMEOUT: Duration = Duration::from_secs(60);
+
+/// Default for [`Config::keep_alive_interval`] (20 seconds).
+pub const DEFAULT_KEEP_ALIVE_INTERVAL: Duration = Duration::from_secs(20);
+
+/// Default for [`Config::upnp_lease_duration`] (2 minutes).
+pub const DEFAULT_UPNP_LEASE_DURATION: Duration = Duration::from_secs(120);
+
+/// Default for [`Config::min_retry_duration`] (30 seconds).
+pub const DEFAULT_MIN_RETRY_DURATION: Duration = Duration::from_secs(30);
+
 /// QuicP2p configurations
 #[derive(Clone, Debug, Default, Serialize, Deserialize, Eq, PartialEq, StructOpt)]
 pub struct Config {
     /// Specify if port forwarding via UPnP should be done or not. This can be set to false if the network
     /// is run locally on the network loopback or on a local area network.
     #[structopt(long)]
     pub forward_port: bool,
+
     /// External port number assigned to the socket address of the program.
     /// If this is provided, QP2p considers that the local port provided has been mapped to the
     /// provided external port number and automatic port forwarding will be skipped.
     #[structopt(long)]
     pub external_port: Option<u16>,
+
     /// External IP address of the computer on the WAN. This field is mandatory if the node is the genesis node and
     /// port forwarding is not available. In case of non-genesis nodes, the external IP address will be resolved
     /// using the Echo service.
     #[structopt(long)]
     pub external_ip: Option<IpAddr>,
-    /// If we hear nothing from the peer in the given interval we declare it offline to us. If none
-    /// supplied we'll default to the documented constant.
+
+    /// How long to wait to hear from a peer before timing out a connection.
     ///
-    /// The interval is in milliseconds. A value of 0 disables this feature.
-    #[structopt(long)]
-    pub idle_timeout_msec: Option<u64>,
-    /// Interval to send keep-alives if we are idling so that the peer does not disconnect from us
-    /// declaring us offline. If none is supplied we'll default to the documented constant.
+    /// In the absence of any keep-alive messages, connections will be closed if they remain idle
+    /// for at least this duration.
     ///
-    /// The interval is in milliseconds. A value of 0 disables this feature.
-    #[structopt(long)]
-    pub keep_alive_interval_msec: Option<u32>,
-    /// Duration of a UPnP port mapping.
-    #[structopt(long)]
-    pub upnp_lease_duration: Option<u32>,
+    /// If unspecified, this will default to [`DEFAULT_IDLE_TIMEOUT`].
+    #[serde(default)]
+    #[structopt(long, parse(try_from_str = parse_millis), value_name = "MILLIS")]
+    pub idle_timeout: Option<Duration>,
+
+    /// Interval at which to send keep-alives to maintain otherwise idle connections.
+    ///
+    /// Keep-alives prevent otherwise idle connections from timing out.
+    ///
+    /// If unspecified, this will default to [`DEFAULT_KEEP_ALIVE_INTERVAL`].
+    #[serde(default)]
+    #[structopt(long, parse(try_from_str = parse_millis), value_name = "MILLIS")]
+    pub keep_alive_interval: Option<Duration>,
+
+    /// How long UPnP port mappings will last.
+    ///
+    /// Note that UPnP port mappings will be automatically renewed on this interval.
+    ///
+    /// If unspecified, this will default to [`DEFAULT_UPNP_LEASE_DURATION`], which should be
+    /// suitable in most cases but some routers may clear UPnP port mapping more frequently.
+    #[serde(default)]
+    #[structopt(long, parse(try_from_str = parse_millis), value_name = "MILLIS")]
+    pub upnp_lease_duration: Option<Duration>,
+
     /// How long to retry establishing connections and sending messages.
     ///
-    /// The duration is in milliseconds. Setting this to 0 will effectively disable retries.
-    #[structopt(long, default_value = "30000")]
-    pub retry_duration_msec: u64,
+    /// Retrying will continue for *at least* this duration, but potentially longer as an
+    /// in-progress back-off delay will not be interrupted.
+    ///
+    /// If unspecified, this will default to [`DEFAULT_MIN_RETRY_DURATION`].
+    #[serde(default)]
+    #[structopt(long, parse(try_from_str = parse_millis), value_name = "MILLIS")]
+    pub min_retry_duration: Option<Duration>,
+}
+
+fn parse_millis(millis: &str) -> Result<Duration, std::num::ParseIntError> {
+    Ok(Duration::from_millis(millis.parse()?))
 }
 
 /// Config that has passed validation.
@@ -63,8 +105,8 @@ pub(crate) struct InternalConfig {
     pub(crate) forward_port: bool,
     pub(crate) external_port: Option<u16>,
     pub(crate) external_ip: Option<IpAddr>,
-    pub(crate) upnp_lease_duration: u32,
-    pub(crate) retry_duration_msec: u64,
+    pub(crate) upnp_lease_duration: Duration,
+    pub(crate) min_retry_duration: Duration,
 }
 
 /// To be used to read and write our certificate and private key to disk esp. as a part of our

src/endpoint.rs

@@ -635,7 +635,7 @@ impl<I: ConnId> Endpoint<I> {
         Fut: futures::Future<Output = Result<R, backoff::Error<E>>>,
     {
         let backoff = ExponentialBackoff {
-            max_elapsed_time: Some(Duration::from_millis(self.qp2p_config.retry_duration_msec)),
+            max_elapsed_time: Some(self.qp2p_config.min_retry_duration),
             ..Default::default()
         };
         retry(backoff, op)

src/igd.rs

@@ -18,14 +18,18 @@ use tracing::{debug, info, warn};
 /// Automatically forwards a port and setups a tokio task to renew it periodically.
 pub(crate) async fn forward_port(
     local_addr: SocketAddr,
-    lease_duration: u32,
+    lease_interval: Duration,
     mut termination_rx: Receiver<()>,
 ) -> Result<u16> {
-    let igd_res = add_port(local_addr, lease_duration).await;
+    // Cap `lease_interval` at `u32::MAX` seconds due to limits on the IGD API. Since this is an
+    // outrageous length of time (~136 years) we just do so silently.
+    let lease_interval = lease_interval.min(Duration::from_secs(u32::MAX.into()));
+    let lease_interval_u32 = lease_interval.as_secs() as u32;
+
+    let igd_res = add_port(local_addr, lease_interval_u32).await;
 
     if let Ok(ext_port) = &igd_res {
         // Start a tokio task to renew the lease periodically.
-        let lease_interval = Duration::from_secs(lease_duration.into());
         let ext_port = *ext_port;
         let _ = tokio::spawn(async move {
             let mut timer = time::interval_at(Instant::now() + lease_interval, lease_interval);
@@ -37,7 +41,7 @@ pub(crate) async fn forward_port(
                 }
                 debug!("Renewing IGD lease for port {}", local_addr);
 
-                let renew_res = renew_port(local_addr, ext_port, lease_duration).await;
+                let renew_res = renew_port(local_addr, ext_port, lease_interval_u32).await;
                 match renew_res {
                     Ok(()) => {}
                     Err(e) => {

src/lib.rs

@@ -48,7 +48,7 @@
 )]
 
 mod api;
-mod config;
+pub mod config;
 mod connection_deduplicator;
 mod connection_pool;
 mod connections;

src/peer_config.rs

@@ -10,47 +10,28 @@
 use crate::Result;
 use std::{sync::Arc, time::Duration};
 
-/// Default interval within which if we hear nothing from the peer we declare it offline to us.
-///
-/// This is based on average time in which routers would close the UDP mapping to the peer if they
-/// see no conversation between them.
-///
-/// The value is in milliseconds.
-pub(crate) const DEFAULT_IDLE_TIMEOUT_MSEC: u64 = 60_000; // 60secs
-/// Default Interval to send keep-alives if we are idling so that the peer does not disconnect from
-/// us declaring us offline. If none is supplied we'll default to the documented constant.
-///
-/// The value is in milliseconds.
-pub(crate) const DEFAULT_KEEP_ALIVE_INTERVAL_MSEC: u32 = 20_000; // 20secs
-
 pub(crate) fn new_client_cfg(
-    idle_timeout_msec: u64,
-    keep_alive_interval_msec: u32,
+    idle_timeout: Duration,
+    keep_alive_interval: Duration,
 ) -> Result<quinn::ClientConfig> {
     let mut config = quinn::ClientConfig::default();
     let crypto_cfg = Arc::make_mut(&mut config.crypto);
     crypto_cfg
         .dangerous()
         .set_certificate_verifier(SkipServerVerification::new());
-    config.transport = Arc::new(new_transport_cfg(
-        idle_timeout_msec,
-        keep_alive_interval_msec,
-    ));
+    config.transport = Arc::new(new_transport_cfg(idle_timeout, keep_alive_interval));
     Ok(config)
 }
 
 pub(crate) fn new_our_cfg(
-    idle_timeout_msec: u64,
-    keep_alive_interval_msec: u32,
+    idle_timeout: Duration,
+    keep_alive_interval: Duration,
     our_cert: quinn::Certificate,
     our_key: quinn::PrivateKey,
 ) -> Result<quinn::ServerConfig> {
     let mut our_cfg_builder = {
         let mut our_cfg = quinn::ServerConfig::default();
-        our_cfg.transport = Arc::new(new_transport_cfg(
-            idle_timeout_msec,
-            keep_alive_interval_msec,
-        ));
+        our_cfg.transport = Arc::new(new_transport_cfg(idle_timeout, keep_alive_interval));
 
         quinn::ServerConfigBuilder::new(our_cfg)
     };
@@ -61,15 +42,14 @@ pub(crate) fn new_our_cfg(
 }
 
 fn new_transport_cfg(
-    idle_timeout_msec: u64,
-    keep_alive_interval_msec: u32,
+    idle_timeout: Duration,
+    keep_alive_interval: Duration,
 ) -> quinn::TransportConfig {
     let mut transport_config = quinn::TransportConfig::default();
     let _ = transport_config
-        .max_idle_timeout(Some(Duration::from_millis(idle_timeout_msec)))
+        .max_idle_timeout(Some(idle_timeout))
         .unwrap_or(&mut quinn::TransportConfig::default());
-    let _ = transport_config
-        .keep_alive_interval(Some(Duration::from_millis(keep_alive_interval_msec.into())));
+    let _ = transport_config.keep_alive_interval(Some(keep_alive_interval));
     transport_config
 }
 

src/tests/mod.rs

@@ -10,7 +10,10 @@
 use crate::{Config, ConnId, QuicP2p};
 use anyhow::Result;
 use bytes::Bytes;
-use std::net::{Ipv4Addr, SocketAddr};
+use std::{
+    net::{Ipv4Addr, SocketAddr},
+    time::Duration,
+};
 use tiny_keccak::{Hasher, Sha3};
 
 /// SHA3-256 hash digest.
@@ -31,7 +34,7 @@ pub(crate) fn new_qp2p() -> Result<QuicP2p<[u8; 32]>> {
         // turn down the retry duration - we won't live forever
         // note that this would just limit retries, UDP connection attempts seem to take 60s to
         // timeout
-        retry_duration_msec: 500,
+        min_retry_duration: Duration::from_millis(500).into(),
         ..Config::default()
     })?;