From 06ce2eb7dd6b9ad9327dc786c5db207b22123278 Mon Sep 17 00:00:00 2001 From: Tatsuya Kawano Date: Sat, 4 Sep 2021 17:42:52 +0800 Subject: [PATCH 1/4] Add `Send` and `'static` bounds to the init futures of `get_or_(try_)insert_with` - Add `Send` and `'static` bounds to the init futures. - Add UI tests to ensure compile failures to occur for misuses of `get_or_(try_)insert_with`. --- .vscode/settings.json | 24 +++++++------ Cargo.toml | 1 + src/future/cache.rs | 9 +++-- src/lib.rs | 9 +++++ tests/ui/get_with_non_send_future.rs | 24 +++++++++++++ tests/ui/get_with_non_send_future.stderr | 12 +++++++ tests/ui/get_with_non_static_future.rs | 25 +++++++++++++ tests/ui/get_with_non_static_future.stderr | 41 ++++++++++++++++++++++ 8 files changed, 131 insertions(+), 14 deletions(-) create mode 100644 tests/ui/get_with_non_send_future.rs create mode 100644 tests/ui/get_with_non_send_future.stderr create mode 100644 tests/ui/get_with_non_static_future.rs create mode 100644 tests/ui/get_with_non_static_future.stderr diff --git a/.vscode/settings.json b/.vscode/settings.json index 1ef8f276..9ba3d92d 100644 --- a/.vscode/settings.json +++ b/.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" ] } \ No newline at end of file diff --git a/Cargo.toml b/Cargo.toml index 8e1966bb..45832f08 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -47,6 +47,7 @@ getrandom = "0.2" reqwest = "0.11" skeptic = "0.13" tokio = { version = "1", features = ["rt-multi-thread", "macros" ] } +trybuild = "1.0" [target.'cfg(skeptic)'.build-dependencies] skeptic = "0.13" diff --git a/src/future/cache.rs b/src/future/cache.rs index 36a50521..76c94162 100644 --- a/src/future/cache.rs +++ b/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) -> V { + pub async fn get_or_insert_with(&self, key: K, init: F) -> V + where + F: Future + 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>> where - F: Future>>, + F: Future>> + Send + 'static, { let hash = self.base.hash(&key); let key = Arc::new(key); diff --git a/src/lib.rs b/src/lib.rs index 28886e1d..77a585fd 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -159,3 +159,12 @@ pub mod unsync; pub(crate) mod common; pub use common::error::PredicateError; + +#[cfg(test)] +mod tests { + #[test] + fn ui() { + let t = trybuild::TestCases::new(); + t.compile_fail("tests/ui/*.rs"); + } +} diff --git a/tests/ui/get_with_non_send_future.rs b/tests/ui/get_with_non_send_future.rs new file mode 100644 index 00000000..4746f805 --- /dev/null +++ b/tests/ui/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); +} diff --git a/tests/ui/get_with_non_send_future.stderr b/tests/ui/get_with_non_send_future.stderr new file mode 100644 index 00000000..c658ce32 --- /dev/null +++ b/tests/ui/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` +note: captured value is not `Send` + --> $DIR/get_with_non_send_future.rs:19:13 + | +19 | data1.to_string() + | ^^^^^ has type `Rc` which is not `Send` diff --git a/tests/ui/get_with_non_static_future.rs b/tests/ui/get_with_non_static_future.rs new file mode 100644 index 00000000..9505d252 --- /dev/null +++ b/tests/ui/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); +} diff --git a/tests/ui/get_with_non_static_future.stderr b/tests/ui/get_with_non_static_future.stderr new file mode 100644 index 00000000..402b144a --- /dev/null +++ b/tests/ui/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 | }) + | From 9b5571998792ea81f2f0beb9b664b0a549b12691 Mon Sep 17 00:00:00 2001 From: Tatsuya Kawano Date: Sat, 4 Sep 2021 19:21:46 +0800 Subject: [PATCH 2/4] Add `Send` and `'static` bounds to the init futures of `get_or_(try_)insert_with` Change the UI tests to run only when future feature is enabled. --- src/lib.rs | 5 +++-- tests/ui/{ => future}/get_with_non_send_future.rs | 0 tests/ui/{ => future}/get_with_non_send_future.stderr | 0 tests/ui/{ => future}/get_with_non_static_future.rs | 0 tests/ui/{ => future}/get_with_non_static_future.stderr | 0 5 files changed, 3 insertions(+), 2 deletions(-) rename tests/ui/{ => future}/get_with_non_send_future.rs (100%) rename tests/ui/{ => future}/get_with_non_send_future.stderr (100%) rename tests/ui/{ => future}/get_with_non_static_future.rs (100%) rename tests/ui/{ => future}/get_with_non_static_future.stderr (100%) diff --git a/src/lib.rs b/src/lib.rs index 77a585fd..fa8d5c09 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -162,9 +162,10 @@ pub use common::error::PredicateError; #[cfg(test)] mod tests { + #[cfg(feature = "future")] #[test] - fn ui() { + fn ui_future() { let t = trybuild::TestCases::new(); - t.compile_fail("tests/ui/*.rs"); + t.compile_fail("tests/ui/future/*.rs"); } } diff --git a/tests/ui/get_with_non_send_future.rs b/tests/ui/future/get_with_non_send_future.rs similarity index 100% rename from tests/ui/get_with_non_send_future.rs rename to tests/ui/future/get_with_non_send_future.rs diff --git a/tests/ui/get_with_non_send_future.stderr b/tests/ui/future/get_with_non_send_future.stderr similarity index 100% rename from tests/ui/get_with_non_send_future.stderr rename to tests/ui/future/get_with_non_send_future.stderr diff --git a/tests/ui/get_with_non_static_future.rs b/tests/ui/future/get_with_non_static_future.rs similarity index 100% rename from tests/ui/get_with_non_static_future.rs rename to tests/ui/future/get_with_non_static_future.rs diff --git a/tests/ui/get_with_non_static_future.stderr b/tests/ui/future/get_with_non_static_future.stderr similarity index 100% rename from tests/ui/get_with_non_static_future.stderr rename to tests/ui/future/get_with_non_static_future.stderr From 5ba626e6ae65ce18b0a4dacba8a88f2b89e70d8b Mon Sep 17 00:00:00 2001 From: Tatsuya Kawano Date: Sat, 4 Sep 2021 20:43:30 +0800 Subject: [PATCH 3/4] Update the CI to run trybuild tests only on Rust stable --- .github/workflows/CI.yml | 16 +++++++++++++--- Cargo.toml | 2 ++ README.md | 2 +- src/lib.rs | 11 +++++++++-- 4 files changed, 25 insertions(+), 6 deletions(-) diff --git a/.github/workflows/CI.yml b/.github/workflows/CI.yml index e97d8a32..7c2f70d2 100644 --- a/.github/workflows/CI.yml +++ b/.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,6 +56,8 @@ jobs: with: command: test args: --release + env: + RUSTFLAGS: '--cfg skeptic' - name: Run tests (future) uses: actions-rs/cargo@v1 @@ -66,6 +65,17 @@ jobs: 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 diff --git a/Cargo.toml b/Cargo.toml index 45832f08..29111035 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -47,6 +47,8 @@ getrandom = "0.2" 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] diff --git a/README.md b/README.md index c4b87c0f..a0b29233 100644 --- a/README.md +++ b/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 ``` diff --git a/src/lib.rs b/src/lib.rs index fa8d5c09..bd77a6f6 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -162,9 +162,16 @@ pub use common::error::PredicateError; #[cfg(test)] mod tests { - #[cfg(feature = "future")] + // #[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_future() { + fn ui_trybuild_future() { let t = trybuild::TestCases::new(); t.compile_fail("tests/ui/future/*.rs"); } From 7863b5c3e5272603dca2bae70bdf6a65c8de54ce Mon Sep 17 00:00:00 2001 From: Tatsuya Kawano Date: Sat, 4 Sep 2021 20:56:48 +0800 Subject: [PATCH 4/4] Update the changelog --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4a0fd267..aa77d7e8 100644 --- a/CHANGELOG.md +++ b/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/