Add map like functionality to read guards

[whitfin]

One feature that the sync features of Tokio have is the ability to map guards such as RwLockReadGuard, which makes it possible to (e.g.) return a reference to a value inside a RwLock<HashMap<T>>. Is it possible that something such as this is added to this library?

Tokio docs for this API: https://docs.rs/tokio/1.1.0/tokio/sync/struct.RwLockReadGuard.html#method.map

[taiki-e]

I'm okay with adding this, but there are some safety issues to keep in mind when implementing/reviewing.

Actually, some of the major (Mutex|RwLock)::map implementations were unsound. tokio-rs/tokio#3344, rust-lang/futures-rs#2239, Amanieu/parking_lot#259, Amanieu/parking_lot#258

[notgull]

I think the costs outweigh the benefits in this case, especially given the impossibility of implementing this behavior soundly. Especially since well-written code shouldn't have to use mapped guards.

[touilleMan]

@notgull sorry for commenting on a closed issue but I'm puzzled by your statement:

Especially since well-written code shouldn't have to use mapped guards

td;dr: what do you think is the proper way to avoid avoid using mapped guard when a mutex wraps a structure where a cooking operation should be done when the lock is taken before returning the result to a caller (with lock still held) ?

My use case:

I find often useful to wrap low-level components (such as database connection) into a higher level structures in order to expose only business related methods:

let storage = Storage::start().await;

let mut transaction = storage.start_transaction().await;
transaction.change_balance("alice", 10).await;
transaction.change_balance("bob", -10).await;
transaction.commit().await;

// ...

struct Storage {
  db: AsyncMutex<SqliteConnection>
}
impl Storage {
  pub async fn open() -> Self { ... }
  pub async fn start_transaction(&'a self) -> StorageTransaction<'a> {
    let guard = self.db.lock().await;
    guard.execute("BEGIN TRANSACTION").await;
    StorageTransaction {
      guard
    }
  }
}

struct StorageTransaction<'a> {
  guard: AsyncMutexGuard<'a>
}
impl<'a> StorageTransaction {
  pub async fn change_balance(&'a self, user: &str, amount: isize) -> StorageTransaction<'a> {
    self.guard.execute("INSERT INTO balances(amount) VALUES (amount + ?) WHERE user = ?", amount, user).await
  }
  pub async fn commit(&self) {
    self.guard.execute("COMMIT").await;
  }
}

So far so good, however this falls apart if I replace AsyncMutex<SqliteConnection> by a AsyncMutex<Option<SqliteConnection>> (i.e. the database connection gets closed, typically by a Storage::close(&self) method).

In this case it seems logical to check if the connection is still open once in the start_transaction method, then have all the other change_balance/commit methods work on SqliteConnection (instead of having a Option<SqliteConnection> with the implicit guaranteed it has been checked for None in a previous step).

[notgull]

td;dr: what do you think is the proper way to avoid avoid using mapped guard when a mutex wraps a structure where a cooking operation should be done when the lock is taken before returning the result to a caller (with lock still held) ?

I understand that in the short term mapped mutexes can seem like a solution to a problem, but overall there are much better solutions to this problem! There are two cases:

The first case is where the "operation" in question is a trivial operation, like unwrapping an Option. At this point I'd recommend just unwrap()ing the guard when you need to perform some operation.

impl<'a> StorageTransaction {
  fn guard(&self) -> &SqliteConnection {
    self.guard.as_ref().unwrap()
  }
  pub async fn change_balance(&'a self, user: &str, amount: isize) -> StorageTransaction<'a> {
    self.guard().execute("INSERT INTO balances(amount) VALUES (amount + ?) WHERE user = ?", amount, user).await
  }
  pub async fn commit(&self) {
    self.guard().execute("COMMIT").await;
  }
}

At this point due to panic-branch prediction the compiler will likely optimize out most cases of the match here. If this isn't happening or if you want to be sure that it's optimized, unwrap_unchecked() can be used as well:

impl<'a> StorageTransaction {
  fn guard(&self) -> &SqliteConnection {
    // SAFETY: We know `guard` is `Some`
    unsafe { self.guard.as_ref().unwrap_unchecked() }
  }
}

This involves unsafe code, but mapped guards are also not implemented soundly (see Kimundi/owning-ref-rs#49 and Storyyeller/stable_deref_trait#10, despite the fact that the latter is closed it's still a fundamental issue). So pick your poison, I suppose.

The second is where it's actually a cold operation that can't reasonably be called multiple times. In this case you have to restructure your program. Make the lock, call the cold operation once, and then pass that to your functions. Like so:

async fn main() {
    let my_conn = /* ... */;
    let mut locked_conn = my_conn.lock().await;
    let mut mapped_thing = cold_operation(&mut locked_conn).await; // If it takes a while it should probably be implemented async-ly.
    
    change_balance(&mut mapped_thing).await;
    execute(&mut mapped_thing).await;
}

Unfortunately it's a flaw with Rust as a whole that it's difficult to pass around temporaries like this. However it's the only solution that doesn't open up soundness holes.

[touilleMan]

@notgull Thanks a lot for this detailed answer 😃

As I understand, lock-map is a useful tool but cursed by Rust memory model.

After digging a bit into the link you provided, I stumbled on rust-lang/rfcs#3467
As I understand, once this RFC is implemented it would be possible to have sound lock-map \o/