Is it safe to use RefCell in a data structure accessed with some coarse grained lock?

Question

I'm writing some tokio async code. I have a multi-index data structure keeping users where I want to guard them with a coarse-grained lock (contrary to per-object lock). I have something like this:

use tokio::sync::RwLock;

struct User {
  id: u64,
  name: String,
}

// This class is not thread-safe.
struct UserDb {
  by_id: HashMap<u64, Arc<RefCell<User>>>,
  by_name: HashMap<String, Arc<RefCell<User>>>,
}

impl UserDb {
  pub fn add_user(&mut self, name: String) -> Result<(), Error> {
    // ...
  }
}

// This class is thread-safe.
struct AsyncDb {
  users: RwLock<UserDb>,
}

impl AsyncDb {
  pub async fn add_user(&self, name: String) -> Result<(), Error> {
    self.users.write().await.add_user(name)
  }
}

// QUESTION: Are these safe?
unsafe impl Send for AsyncDb {}
unsafe impl Sync for AsyncDb {}

Without Send and Sync traits at the end, the compiler complains RefCell<User> isn't Send and Sync (reasonably so), and hence not safe to access/modify through AsyncDb::add_user.

My solution was to implement Send and Sync for the data structure since there is a coarse grained lock in AsyncDb around UserDb, which contains said RefCells.

Is this a correct solution? Does it violate any invariants? Is there a better way to handle this?

Note: Rust beginner here. I likely have many conceptual gaps, so please call it out if things don't make sense.

Answer 1

This is almost certainly not sound unless you only take write locks even when reading from users.

The RefCell::borrow*() functions are not thread-safe because they do not maintain the internal refcount atomically. This means using borrow() to read from a RefCell<User> when only guarded by a read lock is unsound.

If you are already sold on this particular design, I would strongly recommend replacing your RwLock with a Mutex, as read locks will be almost completely useless.

Replacing RwLock with Mutex will make your type automatically implement Sync but it still won't implement Send. If you keep your unsafe impl Send in this case then you need to be very careful with the Arcs. You must never:

• Return an Arc<RefCell<User>> or a reference to one either directly or embedded in another value as that will allow the caller to clone their own Arc that they can use to manipulate the RefCell without holding a lock.
• Send an Arc<RefCell<User>> or reference to one to another thread/task via thread::spawn(), tokio::spawn(), tokio::spawn_blocking(), sending one on a channel, and so on.