Merge pull request #324 from moka-rs/get-timestamp-after-locking

Get the last modified time for upsert after acquiring key level lock

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/

src/future/base_cache.rs

@@ -479,7 +479,6 @@ where
     ) -> (WriteOp<K, V>, 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

src/policy.rs

@@ -86,43 +86,52 @@ pub trait Expiry<K, V> {
     ///
     /// - `key` &mdash; A reference to the key of the entry.
     /// - `value` &mdash; A reference to the value of the entry.
-    /// - `current_time` &mdash; The current instant.
+    /// - `created_at` &mdash; 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<Duration>` is used to set the expiration time of the
+    /// entry.
+    ///
+    /// - Returning `Some(duration)` &mdash; The expiration time is set to
+    ///   `created_at + duration`.
+    /// - Returning `None` &mdash; 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
     ///
     /// When the cache is configured with `time_to_live` and/or `time_to_idle`
     /// 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<Duration> {
+    fn expire_after_create(&self, key: &K, value: &V, created_at: Instant) -> Option<Duration> {
         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` &mdash; A reference to the key of the entry.
     /// - `value` &mdash; A reference to the value of the entry.
-    /// - `current_time` &mdash; The current instant.
-    /// - `current_duration` &mdash; The remaining duration until the entry expires.
-    /// - `last_modified_at` &mdash; The instant when the entry was created or
-    ///   updated.
+    /// - `read_at` &mdash; The time when this entry was read.
+    /// - `duration_until_expiry` &mdash; The remaining duration until the entry
+    ///   expires. (Calculated by `expiration_time - read_at`)
+    /// - `last_modified_at` &mdash; The time when this entry was created or updated.
+    ///
+    /// # Return value
     ///
-    /// # Returning `None` or `current_duration`
+    /// The returned `Option<Duration>` 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)` &mdash; The expiration time is set to
+    ///   `read_at + duration`.
+    /// - Returning `None` &mdash; 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<K, V> {
     ///
     /// - 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<Duration>,
+        read_at: Instant,
+        duration_until_expiry: Option<Duration>,
         last_modified_at: Instant,
     ) -> Option<Duration> {
-        current_duration
+        duration_until_expiry
     }
 
     /// Specifies that the entry should be automatically removed from the cache once
@@ -154,15 +163,20 @@ pub trait Expiry<K, V> {
     ///
     /// - `key` &mdash; A reference to the key of the entry.
     /// - `value` &mdash; A reference to the value of the entry.
-    /// - `current_time` &mdash; The current instant.
-    /// - `current_duration` &mdash; The remaining duration until the entry expires.
+    /// - `updated_at` &mdash; The time when this entry was updated.
+    /// - `duration_until_expiry` &mdash; The remaining duration until the entry
+    ///   expires. (Calculated by `expiration_time - updated_at`)
+    ///
+    /// # Return value
     ///
-    /// # Returning `None` or `current_duration`
+    /// The returned `Option<Duration>` 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)` &mdash; The expiration time is set to
+    ///   `updated_at + duration`.
+    /// - Returning `None` &mdash; 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<K, V> {
     ///
     /// - 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<Duration>,
+        updated_at: Instant,
+        duration_until_expiry: Option<Duration>,
     ) -> Option<Duration> {
-        current_duration
+        duration_until_expiry
     }
 }
 

src/sync_base/base_cache.rs

@@ -480,7 +480,6 @@ where
         hash: u64,
         value: V,
     ) -> (WriteOp<K, V>, 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