From ffb71de738a22a32df99de13446a7070d027a3e8 Mon Sep 17 00:00:00 2001 From: Tatsuya Kawano Date: Sat, 9 Sep 2023 19:44:30 +0800 Subject: [PATCH 1/3] Fix an issue that the last modified time might be get a little too early if eviction listener is enabled --- src/future/base_cache.rs | 3 ++- src/sync_base/base_cache.rs | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/src/future/base_cache.rs b/src/future/base_cache.rs index 2b5e5bf9..ccbfc982 100644 --- a/src/future/base_cache.rs +++ b/src/future/base_cache.rs @@ -479,7 +479,6 @@ where ) -> (WriteOp, Instant) { self.retry_interrupted_ops().await; - let ts = self.current_time_from_expiration_clock(); let weight = self.inner.weigh(&key, &value); let op_cnt1 = Arc::new(AtomicU8::new(0)); let op_cnt2 = Arc::clone(&op_cnt1); @@ -494,6 +493,8 @@ where None }; + let ts = self.current_time_from_expiration_clock(); + // Since the cache (cht::SegmentedHashMap) employs optimistic locking // strategy, insert_with_or_modify() may get an insert/modify operation // conflicted with other concurrent hash table operations. In that case, it diff --git a/src/sync_base/base_cache.rs b/src/sync_base/base_cache.rs index 5f39c43b..acfc7515 100644 --- a/src/sync_base/base_cache.rs +++ b/src/sync_base/base_cache.rs @@ -480,7 +480,6 @@ where hash: u64, value: V, ) -> (WriteOp, Instant) { - let ts = self.current_time_from_expiration_clock(); let weight = self.inner.weigh(&key, &value); let op_cnt1 = Rc::new(AtomicU8::new(0)); let op_cnt2 = Rc::clone(&op_cnt1); @@ -491,6 +490,8 @@ where let kl = self.maybe_key_lock(&key); let _klg = &kl.as_ref().map(|kl| kl.lock()); + let ts = self.current_time_from_expiration_clock(); + // Since the cache (cht::SegmentedHashMap) employs optimistic locking // strategy, insert_with_or_modify() may get an insert/modify operation // conflicted with other concurrent hash table operations. In that case, it From 70880ed39d33e1e0d9432cd2f22cb56cc43b7b41 Mon Sep 17 00:00:00 2001 From: Tatsuya Kawano Date: Sat, 9 Sep 2023 19:46:11 +0800 Subject: [PATCH 2/3] Update the doc for `Expiry` trait for clarity --- src/policy.rs | 76 ++++++++++++++++++++++++++++++--------------------- 1 file changed, 45 insertions(+), 31 deletions(-) diff --git a/src/policy.rs b/src/policy.rs index 155ec3da..2856df22 100644 --- a/src/policy.rs +++ b/src/policy.rs @@ -86,12 +86,17 @@ pub trait Expiry { /// /// - `key` — A reference to the key of the entry. /// - `value` — A reference to the value of the entry. - /// - `current_time` — The current instant. + /// - `created_at` — The time when this entry was inserted. /// - /// # Returning `None` + /// # Return value /// - /// - Returning `None` indicates no expiration for the entry. - /// - The default implementation returns `None`. + /// The returned `Option` is used to set the expiration time of the + /// entry. + /// + /// - Returning `Some(duration)` — The expiration time is set to + /// `created_at + duration`. + /// - Returning `None` — The expiration time is cleared (no expiration). + /// - This is the value that the default implementation returns. /// /// # Notes on `time_to_live` and `time_to_idle` policies /// @@ -99,30 +104,34 @@ pub trait Expiry { /// policies, the entry will be evicted after the earliest of the expiration time /// returned by this expiry, the `time_to_live` and `time_to_idle` policies. #[allow(unused_variables)] - fn expire_after_create(&self, key: &K, value: &V, current_time: Instant) -> Option { + fn expire_after_create(&self, key: &K, value: &V, created_at: Instant) -> Option { None } /// Specifies that the entry should be automatically removed from the cache once /// the duration has elapsed after its last read. This method is called for cache - /// read methods such as `get` and `get_with` but only when the key is present - /// in the cache. + /// read methods such as `get` and `get_with` but only when the key is present in + /// the cache. /// /// # Parameters /// /// - `key` — A reference to the key of the entry. /// - `value` — A reference to the value of the entry. - /// - `current_time` — The current instant. - /// - `current_duration` — The remaining duration until the entry expires. - /// - `last_modified_at` — The instant when the entry was created or - /// updated. + /// - `read_at` — The time when this entry was read. + /// - `duration_until_expiry` — The remaining duration until the entry + /// expires. (Calculated by `expiration_time - read_at`) + /// - `last_modified_at` — The time when this entry was created or updated. + /// + /// # Return value /// - /// # Returning `None` or `current_duration` + /// The returned `Option` is used to set the expiration time of the + /// entry. /// - /// - Returning `None` indicates no expiration for the entry. - /// - Returning `current_duration` will not modify the expiration time. - /// - The default implementation returns `current_duration` (not modify the - /// expiration time) + /// - Returning `Some(duration)` — The expiration time is set to + /// `read_at + duration`. + /// - Returning `None` — The expiration time is cleared (no expiration). + /// - Returning `duration_until_expiry` will not modify the expiration time. + /// - This is the value that the default implementation returns. /// /// # Notes on `time_to_live` and `time_to_idle` policies /// @@ -131,18 +140,18 @@ pub trait Expiry { /// /// - The entry will be evicted after the earliest of the expiration time /// returned by this expiry, the `time_to_live` and `time_to_idle` policies. - /// - The `current_duration` takes in account the `time_to_live` and + /// - The `duration_until_expiry` takes in account the `time_to_live` and /// `time_to_idle` policies. #[allow(unused_variables)] fn expire_after_read( &self, key: &K, value: &V, - current_time: Instant, - current_duration: Option, + read_at: Instant, + duration_until_expiry: Option, last_modified_at: Instant, ) -> Option { - current_duration + duration_until_expiry } /// Specifies that the entry should be automatically removed from the cache once @@ -154,15 +163,20 @@ pub trait Expiry { /// /// - `key` — A reference to the key of the entry. /// - `value` — A reference to the value of the entry. - /// - `current_time` — The current instant. - /// - `current_duration` — The remaining duration until the entry expires. + /// - `updated_at` — The time when this entry was updated. + /// - `duration_until_expiry` — The remaining duration until the entry + /// expires. (Calculated by `expiration_time - updated_at`) + /// + /// # Return value /// - /// # Returning `None` or `current_duration` + /// The returned `Option` is used to set the expiration time of the + /// entry. /// - /// - Returning `None` indicates no expiration for the entry. - /// - Returning `current_duration` will not modify the expiration time. - /// - The default implementation returns `current_duration` (not modify the - /// expiration time) + /// - Returning `Some(duration)` — The expiration time is set to + /// `updated_at + duration`. + /// - Returning `None` — The expiration time is cleared (no expiration). + /// - Returning `duration_until_expiry` will not modify the expiration time. + /// - This is the value that the default implementation returns. /// /// # Notes on `time_to_live` and `time_to_idle` policies /// @@ -171,17 +185,17 @@ pub trait Expiry { /// /// - The entry will be evicted after the earliest of the expiration time /// returned by this expiry, the `time_to_live` and `time_to_idle` policies. - /// - The `current_duration` takes in account the `time_to_live` and + /// - The `duration_until_expiry` takes in account the `time_to_live` and /// `time_to_idle` policies. #[allow(unused_variables)] fn expire_after_update( &self, key: &K, value: &V, - current_time: Instant, - current_duration: Option, + updated_at: Instant, + duration_until_expiry: Option, ) -> Option { - current_duration + duration_until_expiry } } From 650be41be8894770a599910e2e34f577cf5048b7 Mon Sep 17 00:00:00 2001 From: Tatsuya Kawano Date: Sat, 9 Sep 2023 20:01:03 +0800 Subject: [PATCH 3/3] Update the change log --- CHANGELOG.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 52363e05..5bd0bcf4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -29,6 +29,11 @@ Please read the [MIGRATION-GUIDE.md][migration-guide-v012] for more details. caches ([#316][gh-pull-0316]). - Improved async cancellation safety of `future::Cache`. ([#309][gh-pull-0309]) +### Fixed + +- Fixed a bug that an internal `do_insert_with_hash` method gets the current + `Instant` too early when eviction listener is enabled. ([#322][gh-issue-0322]) + ## Version 0.11.3 @@ -696,6 +701,7 @@ The minimum supported Rust version (MSRV) is now 1.51.0 (Mar 25, 2021). [gh-Swatinem]: https://github.com/Swatinem [gh-tinou98]: https://github.com/tinou98 +[gh-issue-0322]: https://github.com/moka-rs/moka/issues/322/ [gh-issue-0255]: https://github.com/moka-rs/moka/issues/255/ [gh-issue-0252]: https://github.com/moka-rs/moka/issues/252/ [gh-issue-0243]: https://github.com/moka-rs/moka/issues/243/