Merge pull request #32 from moka-rs/extra-bounds-on-future

Add `Send` and `'static` bounds to the init futures of `get_or_(try_)insert_with`

.github/workflows/CI.yml

@@ -15,9 +15,6 @@ on:
     # Run against the last commit on the default branch on Friday at 8pm (UTC?)
     - cron:  '0 20 * * 5'
 
-env:
-  RUSTFLAGS: '--cfg skeptic'
-
 jobs:
   test:
     runs-on: ubuntu-latest
@@ -59,13 +56,26 @@ jobs:
         with:
           command: test
           args: --release
+        env:
+          RUSTFLAGS: '--cfg skeptic'
 
       - name: Run tests (future)
         uses: actions-rs/cargo@v1
         if: ${{ matrix.rust != '1.45.2' }}
         with:
           command: test
           args: --release --features future
+        env:
+          RUSTFLAGS: '--cfg skeptic'
+
+      - name: Run UI tests (future, trybuild)
+        uses: actions-rs/cargo@v1
+        if: ${{ matrix.rust == 'stable' }}
+        with:
+          command: test
+          args: ui_trybuild --release --features future
+        env:
+          RUSTFLAGS: '--cfg trybuild'
 
       - name: Run Rustfmt
         uses: actions-rs/cargo@v1

.vscode/settings.json

@@ -1,38 +1,40 @@
 {
     "rust-analyzer.cargo.features": ["future"],
     "cSpell.words": [
-        "CLFU",
-        "Deque",
-        "Deques",
-        "Hasher",
-        "Kawano",
-        "MSRV",
-        "Moka",
-        "RUSTFLAGS",
-        "Ristretto",
-        "Tatsuya",
-        "Upsert",
         "actix",
         "ahash",
         "benmanes",
+        "CLFU",
         "clippy",
         "cpus",
         "deqs",
+        "Deque",
+        "Deques",
+        "else's",
         "getrandom",
+        "Hasher",
+        "Kawano",
+        "Moka",
         "mpsc",
+        "MSRV",
         "nanos",
         "nocapture",
         "peekable",
         "preds",
         "reqwest",
+        "Ristretto",
         "runtimes",
         "rustdoc",
+        "RUSTFLAGS",
         "rustfmt",
         "semver",
         "structs",
+        "Tatsuya",
         "thiserror",
         "toolchain",
+        "trybuild",
         "unsync",
+        "Upsert",
         "usize"
     ]
 }

CHANGELOG.md

@@ -4,6 +4,9 @@
 
 ### Fixed
 
+- Fix `get_or_insert_with` and `get_or_try_insert_with` methods in `future::Cache`
+  can lead undefined behavior as accepting `init` future that is not `Send` or
+  `'static`. ([#31][gh-issue-0031])
 - Fix `usize` overflow on big cache capacity. ([#28][gh-pull-0028])
 
 ### Added
@@ -93,6 +96,7 @@
 
 [caffeine-git]: https://github.com/ben-manes/caffeine
 
+[gh-issue-0030]: https://github.com/moka-rs/moka/issues/30/
 [gh-pull-0030]: https://github.com/moka-rs/moka/pull/30/
 [gh-pull-0028]: https://github.com/moka-rs/moka/pull/28/
 [gh-pull-0022]: https://github.com/moka-rs/moka/pull/22/

Cargo.toml

@@ -48,5 +48,8 @@ reqwest = "0.11"
 skeptic = "0.13"
 tokio = { version = "1", features = ["rt-multi-thread", "macros" ] }
 
+[target.'cfg(trybuild)'.dev-dependencies]
+trybuild = "1.0"
+
 [target.'cfg(skeptic)'.build-dependencies]
 skeptic = "0.13"

README.md

@@ -342,7 +342,7 @@ To run all tests including `future` feature and doc tests on the README, use the
 following command:
 
 ```console
-$ RUSTFLAGS='--cfg skeptic' cargo test --all-features
+$ RUSTFLAGS='--cfg skeptic -cfg trybuild' cargo test --all-features
 ```
 
 

src/future/cache.rs

@@ -307,7 +307,7 @@ where
     ///                 // will call `get_or_insert_with` at the same time, the `init` async
     ///                 // block must be resolved only once.
     ///                 let value = my_cache
-    ///                     .get_or_insert_with("key1", async {
+    ///                     .get_or_insert_with("key1", async move {
     ///                         println!("Task {} inserting a value.", task_id);
     ///                         Arc::new(vec![0u8; TEN_MIB])
     ///                     })
@@ -344,7 +344,10 @@ where
     /// Task 2 got the value. (len: 10485760)
     /// ```
     ///
-    pub async fn get_or_insert_with(&self, key: K, init: impl Future<Output = V>) -> V {
+    pub async fn get_or_insert_with<F>(&self, key: K, init: F) -> V
+    where
+        F: Future<Output = V> + Send + 'static,
+    {
         let hash = self.base.hash(&key);
         let key = Arc::new(key);
         self.get_or_insert_with_hash_and_fun(key, hash, init).await
@@ -445,7 +448,7 @@ where
         init: F,
     ) -> Result<V, Arc<Box<dyn Error + Send + Sync + 'static>>>
     where
-        F: Future<Output = Result<V, Box<dyn Error + Send + Sync + 'static>>>,
+        F: Future<Output = Result<V, Box<dyn Error + Send + Sync + 'static>>> + Send + 'static,
     {
         let hash = self.base.hash(&key);
         let key = Arc::new(key);

src/lib.rs

@@ -159,3 +159,20 @@ pub mod unsync;
 pub(crate) mod common;
 
 pub use common::error::PredicateError;
+
+#[cfg(test)]
+mod tests {
+    // #[cfg(trybuild)]
+    // #[test]
+    // fn ui_trybuild() {
+    //     let t = trybuild::TestCases::new();
+    //     t.compile_fail("tests/ui/default/*.rs");
+    // }
+
+    #[cfg(all(trybuild, feature = "future"))]
+    #[test]
+    fn ui_trybuild_future() {
+        let t = trybuild::TestCases::new();
+        t.compile_fail("tests/ui/future/*.rs");
+    }
+}

tests/ui/future/get_with_non_send_future.rs

@@ -0,0 +1,24 @@
+// https://github.com/moka-rs/moka/issues/31
+
+use moka::future::Cache;
+use std::rc::Rc;
+
+#[tokio::main]
+async fn main() {
+    let cache: Cache<_, String> = Cache::new(100);
+
+    // Rc is !Send.
+    let data = Rc::new("zero".to_string());
+    let data1 = Rc::clone(&data);
+
+    cache
+        .get_or_insert_with(0, async move {
+            // A data race may occur. 
+            // The async block can be executed by a different thread
+            // but Rc's internal reference counters are not thread safe.
+            data1.to_string()
+        })
+        .await;
+
+    println!("{:?}", data);
+}

tests/ui/future/get_with_non_send_future.stderr

@@ -0,0 +1,12 @@
+error: future cannot be sent between threads safely
+  --> $DIR/get_with_non_send_future.rs:15:10
+   |
+15 |         .get_or_insert_with(0, async move {
+   |          ^^^^^^^^^^^^^^^^^^ future created by async block is not `Send`
+   |
+   = help: within `impl Future`, the trait `Send` is not implemented for `Rc<String>`
+note: captured value is not `Send`
+  --> $DIR/get_with_non_send_future.rs:19:13
+   |
+19 |             data1.to_string()
+   |             ^^^^^ has type `Rc<String>` which is not `Send`

tests/ui/future/get_with_non_static_future.rs

@@ -0,0 +1,25 @@
+// https://github.com/moka-rs/moka/issues/31
+
+use moka::future::Cache;
+
+#[tokio::main]
+async fn main() {
+    let cache: Cache<_, String> = Cache::new(100);
+
+    let data = "zero".to_string();
+    {
+        // Not 'static.
+        let data_ref = &data;
+
+        cache
+            .get_or_insert_with(0, async {
+                // This may become a dangling pointer.
+                // The async block can be executed by a different thread so
+                // the captured reference `data_ref` may outlive its value.
+                data_ref.to_string()
+            })
+            .await;
+    }
+
+    println!("{:?}", data);
+}

tests/ui/future/get_with_non_static_future.stderr

@@ -0,0 +1,41 @@
+error[E0597]: `data` does not live long enough
+  --> $DIR/get_with_non_static_future.rs:12:24
+   |
+12 |           let data_ref = &data;
+   |                          ^^^^^ borrowed value does not live long enough
+...
+15 |               .get_or_insert_with(0, async {
+   |  ____________________________________-
+16 | |                 // This may become a dangling pointer.
+17 | |                 // The async block can be executed by a different thread so
+18 | |                 // the captured reference `data_ref` may outlive its value.
+19 | |                 data_ref.to_string()
+20 | |             })
+   | |_____________- argument requires that `data` is borrowed for `'static`
+...
+25 |   }
+   |   - `data` dropped here while still borrowed
+
+error[E0373]: async block may outlive the current function, but it borrows `data_ref`, which is owned by the current function
+  --> $DIR/get_with_non_static_future.rs:15:42
+   |
+15 |               .get_or_insert_with(0, async {
+   |  __________________________________________^
+16 | |                 // This may become a dangling pointer.
+17 | |                 // The async block can be executed by a different thread so
+18 | |                 // the captured reference `data_ref` may outlive its value.
+19 | |                 data_ref.to_string()
+   | |                 -------- `data_ref` is borrowed here
+20 | |             })
+   | |_____________^ may outlive borrowed value `data_ref`
+   |
+   = note: async blocks are not executed immediately and must either take a reference or ownership of outside variables they use
+help: to force the async block to take ownership of `data_ref` (and any other referenced variables), use the `move` keyword
+   |
+15 |             .get_or_insert_with(0, async move {
+16 |                 // This may become a dangling pointer.
+17 |                 // The async block can be executed by a different thread so
+18 |                 // the captured reference `data_ref` may outlive its value.
+19 |                 data_ref.to_string()
+20 |             })
+   |