Repository: fujiapple852/trippy (language: rust)

Issue:
[BREAKING CHANGE] Remove `Timestamp` from all `DnsEntry` variants



Discussion / hints:

Fix patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index c188188ec..dbe370855 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -13,8 +13,13 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
 ### Changed
 
+- [BREAKING CHANGE] Remove `Timestamp` from all `DnsEntry`
+  variants ([#1296](https://github.com/fujiapple852/trippy/issues/1296))
+
 ### Fixed
 
+- Reverse dns enqueued multiple times when dns-ttl expires ([#1290](https://github.com/fujiapple852/trippy/issues/1290))
+
 ## [0.11.0] - 2024-08-11
 
 ### Added
diff --git a/crates/trippy-dns/src/lazy_resolver.rs b/crates/trippy-dns/src/lazy_resolver.rs
index ad65cba2e..79b799d25 100644
--- a/crates/trippy-dns/src/lazy_resolver.rs
+++ b/crates/trippy-dns/src/lazy_resolver.rs
@@ -116,7 +116,26 @@ mod inner {
     const RESOLVER_QUEUE_TIMEOUT: Duration = Duration::from_millis(10);
 
     /// Alias for a cache of reverse DNS lookup entries.
-    type Cache = Arc<RwLock<HashMap<IpAddr, DnsEntry>>>;
+    type Cache = Arc<RwLock<HashMap<IpAddr, CacheEntry>>>;
+
+    /// A cache entry for a reverse DNS lookup.
+    #[derive(Debug, Clone)]
+    struct CacheEntry {
+        /// The DNS entry to cache.
+        entry: DnsEntry,
+        /// The timestamp of the entry.
+        timestamp: SystemTime,
+    }
+
+    impl CacheEntry {
+        const fn new(entry: DnsEntry, timestamp: SystemTime) -> Self {
+            Self { entry, timestamp }
+        }
+
+        fn set_timestamp(&mut self, timestamp: SystemTime) {
+            self.timestamp = timestamp;
+        }
+    }
 
     #[derive(Clone)]
     enum DnsProvider {
@@ -125,13 +144,13 @@ mod inner {
     }
 
     #[derive(Debug, Clone)]
-    pub struct DnsResolveRequest {
+    struct DnsResolveRequest {
         addr: IpAddr,
         with_asinfo: bool,
     }
 
     /// Resolver implementation.
-    pub struct DnsResolver {
+    pub(super) struct DnsResolver {
         config: Config,
         provider: DnsProvider,
         tx: Sender<DnsResolveRequest>,
@@ -139,7 +158,7 @@ mod inner {
     }
 
     impl DnsResolver {
-        pub fn start(config: Config) -> std::io::Result<Self> {
+        pub(super) fn start(config: Config) -> std::io::Result<Self> {
             let (tx, rx) = bounded(RESOLVER_MAX_QUEUE_SIZE);
             let addr_cache = Arc::new(RwLock::new(HashMap::new()));
 
@@ -180,11 +199,11 @@ mod inner {
             })
         }
 
-        pub const fn config(&self) -> &Config {
+        pub(super) const fn config(&self) -> &Config {
             &self.config
         }
 
-        pub fn lookup(&self, hostname: &str) -> Result<ResolvedIpAddrs> {
+        pub(super) fn lookup(&self, hostname: &str) -> Result<ResolvedIpAddrs> {
             match &self.provider {
                 DnsProvider::TrustDns(resolver) => Ok(resolver
                     .lookup_ip(hostname)
@@ -234,15 +253,20 @@ mod inner {
             .map(ResolvedIpAddrs)
         }
 
-        pub fn reverse_lookup(&self, addr: IpAddr, with_asinfo: bool, lazy: bool) -> DnsEntry {
+        pub(super) fn reverse_lookup(
+            &self,
+            addr: IpAddr,
+            with_asinfo: bool,
+            lazy: bool,
+        ) -> DnsEntry {
             if lazy {
-                self.lazy_reverse_lookup(addr, with_asinfo)
+                self.lazy_reverse_lookup(addr, with_asinfo).entry
             } else {
-                reverse_lookup(&self.provider, addr, with_asinfo)
+                reverse_lookup(&self.provider, addr, with_asinfo).entry
             }
         }
 
-        fn lazy_reverse_lookup(&self, addr: IpAddr, with_asinfo: bool) -> DnsEntry {
+        fn lazy_reverse_lookup(&self, addr: IpAddr, with_asinfo: bool) -> CacheEntry {
             let mut enqueue = false;
             let now = SystemTime::now();
 
@@ -254,17 +278,22 @@ mod inner {
                 .entry(addr)
                 .or_insert_with(|| {
                     enqueue = true;
-                    DnsEntry::Pending(addr, now)
+                    CacheEntry::new(DnsEntry::Pending(addr), now)
                 })
                 .clone();
 
             // If the entry exists but is stale then enqueue it again.  The existing entry will
-            // be returned until it is refreshed.
-            match &dns_entry {
-                DnsEntry::Resolved(_, timestamp)
-                | DnsEntry::NotFound(_, timestamp)
-                | DnsEntry::Failed(_, timestamp) => {
-                    if now.duration_since(*timestamp).unwrap_or_default() > self.config.ttl {
+            // be returned until it is refreshed but with an updated timestamp to prevent it from
+            // being enqueued multiple times.
+            match &dns_entry.entry {
+                DnsEntry::Resolved(_) | DnsEntry::NotFound(_) | DnsEntry::Failed(_) => {
+                    if now.duration_since(dns_entry.timestamp).unwrap_or_default() > self.config.ttl
+                    {
+                        self.addr_cache
+                            .write()
+                            .get_mut(&addr)
+                            .expect("addr must be in cache")
+                            .set_timestamp(now);
                         enqueue = true;
                     }
                 }
@@ -273,13 +302,14 @@ mod inner {
 
             // If the entry exists but has timed out, then set it as DnsEntry::Pending and enqueue
             // it again.
-            if let DnsEntry::Timeout(addr, _) = dns_entry {
+            if let DnsEntry::Timeout(addr) = dns_entry.entry {
                 *self
                     .addr_cache
                     .write()
                     .get_mut(&addr)
-                    .expect("addr must be in cache") = DnsEntry::Pending(addr, now);
-                dns_entry = DnsEntry::Pending(addr, now);
+                    .expect("addr must be in cache") =
+                    CacheEntry::new(DnsEntry::Pending(addr), now);
+                dns_entry = CacheEntry::new(DnsEntry::Pending(addr), now);
                 enqueue = true;
             }
 
@@ -301,8 +331,9 @@ mod inner {
                         .addr_cache
                         .write()
                         .get_mut(&addr)
-                        .expect("addr must be in cache") = DnsEntry::Timeout(addr, now);
-                    DnsEntry::Timeout(addr, now)
+                        .expect("addr must be in cache") =
+                        CacheEntry::new(DnsEntry::Timeout(addr), now);
+                    CacheEntry::new(DnsEntry::Timeout(addr), now)
                 }
             } else {
                 dns_entry
@@ -329,15 +360,17 @@ mod inner {
         }
     }
 
-    fn reverse_lookup(provider: &DnsProvider, addr: IpAddr, with_asinfo: bool) -> DnsEntry {
+    fn reverse_lookup(provider: &DnsProvider, addr: IpAddr, with_asinfo: bool) -> CacheEntry {
         let now = SystemTime::now();
         match &provider {
             DnsProvider::DnsLookup => {
                 // we can't distinguish between a failed lookup or a genuine error, and so we just
                 // assume all failures are `DnsEntry::NotFound`.
                 match dns_lookup::lookup_addr(&addr) {
-                    Ok(dns) => DnsEntry::Resolved(Resolved::Normal(addr, vec![dns]), now),
-                    Err(_) => DnsEntry::NotFound(Unresolved::Normal(addr), now),
+                    Ok(dns) => {
+                        CacheEntry::new(DnsEntry::Resolved(Resolved::Normal(addr, vec![dns])), now)
+                    }
+                    Err(_) => CacheEntry::new(DnsEntry::NotFound(Unresolved::Normal(addr)), now),
                 }
             }
             DnsProvider::TrustDns(resolver) => match resolver.reverse_lookup(addr) {
@@ -352,22 +385,28 @@ mod inner {
                         .collect();
                     if with_asinfo {
                         let as_info = lookup_asinfo(resolver, addr).unwrap_or_default();
-                        DnsEntry::Resolved(Resolved::WithAsInfo(addr, hostnames, as_info), now)
+                        CacheEntry::new(
+                            DnsEntry::Resolved(Resolved::WithAsInfo(addr, hostnames, as_info)),
+                            now,
+                        )
                     } else {
-                        DnsEntry::Resolved(Resolved::Normal(addr, hostnames), now)
+                        CacheEntry::new(DnsEntry::Resolved(Resolved::Normal(addr, hostnames)), now)
                     }
                 }
                 Err(err) => match err.kind() {
                     ResolveErrorKind::NoRecordsFound { .. } => {
                         if with_asinfo {
                             let as_info = lookup_asinfo(resolver, addr).unwrap_or_default();
-                            DnsEntry::NotFound(Unresolved::WithAsInfo(addr, as_info), now)
+                            CacheEntry::new(
+                                DnsEntry::NotFound(Unresolved::WithAsInfo(addr, as_info)),
+                                now,
+                            )
                         } else {
-                            DnsEntry::NotFound(Unresolved::Normal(addr), now)
+                            CacheEntry::new(DnsEntry::NotFound(Unresolved::Normal(addr)), now)
                         }
                     }
-                    ResolveErrorKind::Timeout => DnsEntry::Timeout(addr, now),
-                    _ => DnsEntry::Failed(addr, now),
+                    ResolveErrorKind::Timeout => CacheEntry::new(DnsEntry::Timeout(addr), now),
+                    _ => CacheEntry::new(DnsEntry::Failed(addr), now),
                 },
             },
         }
diff --git a/crates/trippy-dns/src/lib.rs b/crates/trippy-dns/src/lib.rs
index 72a4a32d6..0f97d252a 100644
--- a/crates/trippy-dns/src/lib.rs
+++ b/crates/trippy-dns/src/lib.rs
@@ -34,34 +34,34 @@
 //! loop {
 //!     let entry = resolver.lazy_reverse_lookup_with_asinfo(addr);
 //!     match entry {
-//!         DnsEntry::Pending(ip, timestamp) => {
-//!             println!("lookup of {ip} is pending, sleeping for 1 sec at {timestamp:?}");
+//!         DnsEntry::Pending(ip) => {
+//!             println!("lookup of {ip} is pending, sleeping for 1 sec");
 //!             sleep(Duration::from_secs(1));
 //!         }
-//!         DnsEntry::Resolved(Resolved::Normal(ip, addrs), timestamp) => {
-//!             println!("lookup of {ip} resolved to {addrs:?} at {timestamp:?}");
+//!         DnsEntry::Resolved(Resolved::Normal(ip, addrs)) => {
+//!             println!("lookup of {ip} resolved to {addrs:?}");
 //!             return Ok(());
 //!         }
-//!         DnsEntry::Resolved(Resolved::WithAsInfo(ip, addrs, as_info), timestamp) => {
-//!             println!("lookup of {ip} resolved to {addrs:?} with AS information {as_info:?} at {timestamp:?}");
+//!         DnsEntry::Resolved(Resolved::WithAsInfo(ip, addrs, as_info)) => {
+//!             println!("lookup of {ip} resolved to {addrs:?} with AS information {as_info:?}");
 //!             return Ok(());
 //!         }
-//!         DnsEntry::NotFound(Unresolved::Normal(ip), timestamp) => {
-//!             println!("lookup of {ip} did not match any records at {timestamp:?}");
+//!         DnsEntry::NotFound(Unresolved::Normal(ip)) => {
+//!             println!("lookup of {ip} did not match any records");
 //!             return Ok(());
 //!         }
-//!         DnsEntry::NotFound(Unresolved::WithAsInfo(ip, as_info), timestamp) => {
+//!         DnsEntry::NotFound(Unresolved::WithAsInfo(ip, as_info)) => {
 //!             println!(
-//!                 "lookup of {ip} did not match any records with AS information {as_info:?} at {timestamp:?}"
+//!                 "lookup of {ip} did not match any records with AS information {as_info:?}"
 //!             );
 //!             return Ok(());
 //!         }
-//!         DnsEntry::Timeout(ip, timestamp) => {
-//!             println!("lookup of {ip} timed out at {timestamp:?}");
+//!         DnsEntry::Timeout(ip) => {
+//!             println!("lookup of {ip} timed out");
 //!             return Ok(());
 //!         }
-//!         DnsEntry::Failed(ip, timestamp) => {
-//!             println!("lookup of {ip} failed at {timestamp:?}");
+//!         DnsEntry::Failed(ip) => {
+//!             println!("lookup of {ip} failed");
 //!             return Ok(());
 //!         }
 //!     }
diff --git a/crates/trippy-dns/src/resolver.rs b/crates/trippy-dns/src/resolver.rs
index d9006bb37..b9ce8c442 100644
--- a/crates/trippy-dns/src/resolver.rs
+++ b/crates/trippy-dns/src/resolver.rs
@@ -1,6 +1,5 @@
 use std::fmt::{Display, Formatter};
 use std::net::IpAddr;
-use std::time::SystemTime;
 use thiserror::Error;
 
 /// A DNS resolver.
@@ -83,15 +82,15 @@ impl IntoIterator for ResolvedIpAddrs {
 #[derive(Debug, Clone)]
 pub enum DnsEntry {
     /// The reverse DNS resolution of `IpAddr` is pending.
-    Pending(IpAddr, SystemTime),
+    Pending(IpAddr),
     /// The reverse DNS resolution of `IpAddr` has resolved.
-    Resolved(Resolved, SystemTime),
+    Resolved(Resolved),
     /// The `IpAddr` could not be resolved.
-    NotFound(Unresolved, SystemTime),
+    NotFound(Unresolved),
     /// The reverse DNS resolution of `IpAddr` failed.
-    Failed(IpAddr, SystemTime),
+    Failed(IpAddr),
     /// The reverse DNS resolution of `IpAddr` timed out.
-    Timeout(IpAddr, SystemTime),
+    Timeout(IpAddr),
 }
 
 /// Information about a resolved `IpAddr`.
@@ -145,17 +144,17 @@ impl Display for DnsEntry {
     fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
         #[allow(clippy::match_same_arms)]
         match self {
-            Self::Resolved(Resolved::Normal(_, hosts), _) => write!(f, "{}", hosts.join(" ")),
-            Self::Resolved(Resolved::WithAsInfo(_, hosts, asinfo), _) => {
+            Self::Resolved(Resolved::Normal(_, hosts)) => write!(f, "{}", hosts.join(" ")),
+            Self::Resolved(Resolved::WithAsInfo(_, hosts, asinfo)) => {
                 write!(f, "AS{} {}", asinfo.asn, hosts.join(" "))
             }
-            Self::Pending(ip, _) => write!(f, "{ip}"),
-            Self::Timeout(ip, _) => write!(f, "Timeout: {ip}"),
-            Self::NotFound(Unresolved::Normal(ip), _) => write!(f, "{ip}"),
-            Self::NotFound(Unresolved::WithAsInfo(ip, asinfo), _) => {
+            Self::Pending(ip) => write!(f, "{ip}"),
+            Self::Timeout(ip) => write!(f, "Timeout: {ip}"),
+            Self::NotFound(Unresolved::Normal(ip)) => write!(f, "{ip}"),
+            Self::NotFound(Unresolved::WithAsInfo(ip, asinfo)) => {
                 write!(f, "AS{} {}", asinfo.asn, ip)
             }
-            Self::Failed(ip, _) => write!(f, "Failed: {ip}"),
+            Self::Failed(ip) => write!(f, "Failed: {ip}"),
         }
     }
 }
diff --git a/crates/trippy-tui/src/frontend/render/table.rs b/crates/trippy-tui/src/frontend/render/table.rs
index 05fc3d9ed..83daa106c 100644
--- a/crates/trippy-tui/src/frontend/render/table.rs
+++ b/crates/trippy-tui/src/frontend/render/table.rs
@@ -373,24 +373,24 @@ fn format_address(
 /// Format a `DnsEntry` with or without `AS` information (if available)
 fn format_dns_entry(dns_entry: DnsEntry, lookup_as_info: bool, as_mode: AsMode) -> String {
     match dns_entry {
-        DnsEntry::Resolved(Resolved::Normal(_, hosts), _) => hosts.join(" "),
-        DnsEntry::Resolved(Resolved::WithAsInfo(_, hosts, asinfo), _) => {
+        DnsEntry::Resolved(Resolved::Normal(_, hosts)) => hosts.join(" "),
+        DnsEntry::Resolved(Resolved::WithAsInfo(_, hosts, asinfo)) => {
             if lookup_as_info && !asinfo.asn.is_empty() {
                 format!("{} {}", format_asinfo(&asinfo, as_mode), hosts.join(" "))
             } else {
                 hosts.join(" ")
             }
         }
-        DnsEntry::NotFound(Unresolved::Normal(ip), _) | DnsEntry::Pending(ip, _) => format!("{ip}"),
-        DnsEntry::NotFound(Unresolved::WithAsInfo(ip, asinfo), _) => {
+        DnsEntry::NotFound(Unresolved::Normal(ip)) | DnsEntry::Pending(ip) => format!("{ip}"),
+        DnsEntry::NotFound(Unresolved::WithAsInfo(ip, asinfo)) => {
             if lookup_as_info && !asinfo.asn.is_empty() {
                 format!("{} {}", format_asinfo(&asinfo, as_mode), ip)
             } else {
                 format!("{ip}")
             }
         }
-        DnsEntry::Failed(ip, _) => format!("Failed: {ip}"),
-        DnsEntry::Timeout(ip, _) => format!("Timeout: {ip}"),
+        DnsEntry::Failed(ip) => format!("Failed: {ip}"),
+        DnsEntry::Timeout(ip) => format!("Timeout: {ip}"),
     }
 }
 
@@ -544,10 +544,10 @@ fn format_details(
     let ext = hop.extensions();
     let nat = hop.last_nat_status();
     match dns_entry {
-        DnsEntry::Pending(addr, _) => {
+        DnsEntry::Pending(addr) => {
             fmt_details_line(addr, index, count, None, None, geoip, ext, nat, config)
         }
-        DnsEntry::Resolved(Resolved::WithAsInfo(addr, hosts, asinfo), _) => fmt_details_line(
+        DnsEntry::Resolved(Resolved::WithAsInfo(addr, hosts, asinfo)) => fmt_details_line(
             addr,
             index,
             count,
@@ -558,7 +558,7 @@ fn format_details(
             nat,
             config,
         ),
-        DnsEntry::NotFound(Unresolved::WithAsInfo(addr, asinfo), _) => fmt_details_line(
+        DnsEntry::NotFound(Unresolved::WithAsInfo(addr, asinfo)) => fmt_details_line(
             addr,
             index,
             count,
@@ -569,7 +569,7 @@ fn format_details(
             nat,
             config,
         ),
-        DnsEntry::Resolved(Resolved::Normal(addr, hosts), _) => fmt_details_line(
+        DnsEntry::Resolved(Resolved::Normal(addr, hosts)) => fmt_details_line(
             addr,
             index,
             count,
@@ -580,7 +580,7 @@ fn format_details(
             nat,
             config,
         ),
-        DnsEntry::NotFound(Unresolved::Normal(addr), _) => fmt_details_line(
+        DnsEntry::NotFound(Unresolved::Normal(addr)) => fmt_details_line(
             addr,
             index,
             count,
@@ -591,10 +591,10 @@ fn format_details(
             nat,
             config,
         ),
-        DnsEntry::Failed(ip, _) => {
+        DnsEntry::Failed(ip) => {
             format!("Failed: {ip}")
         }
-        DnsEntry::Timeout(ip, _) => {
+        DnsEntry::Timeout(ip) => {
             format!("Timeout: {ip}")
         }
     }


Test patch: