Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Send + Sync + 'static bounds to K, V and S of sync and future Caches #19

Merged
merged 3 commits into from
Jun 13, 2021

Conversation

tatsuya6502
Copy link
Member

@tatsuya6502 tatsuya6502 commented Jun 13, 2021

This PR adds Send, Sync and 'static bounds to the type parameters K (key), V (value) and S (hasher state) of the following cache implementations:

  • sync::Cache
  • sync::SegmentedCache
  • future::Cache

This will prevent the following usage from drawing undefined behavior:

  • Single thread application using an above thread-safe cache.
  • Store non-Send or non-Sync type (such as std::rc::Rc) as key (K) and/or (Value).
  • Or, store non static reference type as key and/or value.

Until now, storing such key or value types was possible in single thread application (including single-threaded async runtime such as Actix-rt) because above caches did not ask these bounds explicitly.

However doing so is incorrect as it can draw undefined behavior. This is because these caches use multiple threads internally for updating internal data structures and evicting cached values.

use std::{rc::Rc, time::Duration};

// This code will compile, but cat hit an undefined behavior (UB) as breaking
// thread safety guarantee of std::rc::Rc.

// Create a sync::Cache with TTL 1 second.
let cache = moka::sync::CacheBuilder::new(100)
    .time_to_live(Duration::from_secs(1))
    .build();

// Create a Rc, which is !Send and !Sync. (Rc's strong reference count should be 1)
let v = Rc::new("Hello".to_string());

// Insert a clone of RC to the cache. (The strong count should be 2 now)
cache.insert(0, Rc::clone(&v));

// Sleep for 5 seconds.
std::thread::sleep(Duration::from_secs(5));

// While sleeping, the value `v` should have been expired so sync::Cache's
// eviction thread will drop it. Dropping will make the strong count to be
// decremented by 1 but this thread may not see the decrement as Rc is
// neither Send nor Sync.

println!("{}", Rc::strong_count(&v));  // 1 or 2? (*1)

Adding Send, Sync and 'static bounds should prevent the problem by making such code not to compile by type errors.

Note that multi-thread APIs such as std::thread::spawn or multi-threaded async runtime (such as the ones provided by Tokio and async-std) will ask those bounds explicitly. So there should have been no problem with them.


*1: I wrote the code for illustrating purpose. If you actually run the code, it will always prints 2. However, this does not mean you hit the UB, but it is actually an expected behavior. sync::Cache and future::Cache employ concurrent hash table cht::SegmentedHashMap as the main key value storage. cht will not drop a removed value immediately by the spec to provide high concurrency and thread safety. See this issue for more details: Gregory-Meyer/cht#23

@tatsuya6502 tatsuya6502 marked this pull request as ready for review June 13, 2021 14:32
@tatsuya6502
Copy link
Member Author

Adding Send, Sync and 'static bounds should prevent the problem by making such code not to compile by type errors.

Verified that the above code no longer compiles.

error[E0277]: `Rc<String>` cannot be sent between threads safely
 --> examples/rc-ex.rs:8:13
  |
8 | let cache = moka::sync::CacheBuilder::new(100)
  |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `Rc<String>` cannot be sent between threads safely
  |
  = help: the trait `Send` is not implemented for `Rc<String>`
  = note: required by `moka::sync::CacheBuilder::<moka::sync::Cache<K, V>>::new`

error[E0277]: `Rc<String>` cannot be shared between threads safely
 --> examples/rc-ex.rs:8:13
  |
8 | let cache = moka::sync::CacheBuilder::new(100)
  |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `Rc<String>` cannot be shared between threads safely
  |
  = help: the trait `Sync` is not implemented for `Rc<String>`
  = note: required by `moka::sync::CacheBuilder::<moka::sync::Cache<K, V>>::new`

error[E0277]: `Rc<String>` cannot be sent between threads safely
  --> examples/rc-ex.rs:10:6
   |
10 |     .build();
   |      ^^^^^ `Rc<String>` cannot be sent between threads safely
   |
   = help: the trait `Send` is not implemented for `Rc<String>`

error[E0277]: `Rc<String>` cannot be shared between threads safely
  --> examples/rc-ex.rs:10:6
   |
10 |     .build();
   |      ^^^^^ `Rc<String>` cannot be shared between threads safely
   |
   = help: the trait `Sync` is not implemented for `Rc<String>`

error[E0277]: `Rc<String>` cannot be sent between threads safely
  --> examples/rc-ex.rs:16:17
   |
16 | cache.insert(0, Rc::clone(&v));
   |                 ^^^^^^^^^^^^^ `Rc<String>` cannot be sent between threads safely
   |
   = help: the trait `Send` is not implemented for `Rc<String>`

error[E0277]: `Rc<String>` cannot be shared between threads safely
  --> examples/rc-ex.rs:16:17
   |
16 | cache.insert(0, Rc::clone(&v));
   |                 ^^^^^^^^^^^^^ `Rc<String>` cannot be shared between threads safely
   |
   = help: the trait `Sync` is not implemented for `Rc<String>`

error: aborting due to 6 previous errors

For more information about this error, try `rustc --explain E0277`.
error: could not compile `moka`

To learn more, run the command again with --verbose.

Copy link
Member Author

@tatsuya6502 tatsuya6502 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Merging.

@tatsuya6502 tatsuya6502 merged commit e04d2ef into master Jun 13, 2021
@tatsuya6502 tatsuya6502 deleted the add-send-sync-static-bounds branch June 13, 2021 14:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant