Locking code pattern

[bllhastings]

I am looking for advice here. Could someone please tell me if this is right way to structure code in Rust? In particular please look at the open() method which uses locks. The code works but I am looking for validation as to whether this is the right way to code when employing locks.

use std::sync::{Arc, RwLock, RwLockReadGuard, RwLockWriteGuard};
use std::collections::HashMap;

struct Table<'a>
{
    name_ : &'a str
}

impl<'a> Table<'a>
{
    fn new(name : &'a str) -> Self
    {
        return Self {
            name_ : name
        }
    }
    
    fn name(&self) -> &str
    {
        return self.name_;
    }
}

struct TableManager<'a>
{
    tables_ : RwLock<HashMap<&'a str, Arc<Table<'a>>>>
}

impl<'a> TableManager<'a>
{
    fn new() -> Self
    {
        return Self {
            tables_ : RwLock::new(HashMap::new()),
        }    
    }
    
    **fn open(&mut self, name : &'a str) -> Arc<Table>**
    {
        {
            let rd_guard = self.tables_.read().unwrap();
            let table = rd_guard.get(name);
            if let Some(table) = table
            {
                return Arc::clone(&table);
            }    
        }
        {
            let mut wr_guard = self.tables_.write().unwrap();
            let table = wr_guard.get(name);
            if table.is_none()
            {
                let table = Table::new(name);
                wr_guard.insert(name, Arc::new(table));
            }
        }
        let rd_guard = self.tables_.read().unwrap();
        let table = rd_guard.get(name);
        return Arc::clone(&table.unwrap());
    }
}

fn main() -> ()
{
    let mut table_mgr = TableManager::new();
    let table = table_mgr.create_table("System");
    println!("{}", table.name());
}

[alice]

Why are you using an RwLock? It seems like your method is able to be &mut self, so locking shouldn't be necessary.

As an aside, you shouldn't have any lifetimes on your table structs. It wont work unless the strings are compile-time constants. Use String instead of &str.

[bllhastings]

Thanks. Sorry my bad I should have removed the &mut self. Assume it were &self. Does the RwLock usage look correct?

Also could you please elaborate on the incorrect usage of lifetimes on the Table struct?

[H2CO3]

If you are not taking &mut self, then your usage is not correct.

In general, "check-and-create-if-not-exist" patterns are racy. Another, competing thread might grab the lock and write into your table between the releasing of the read guard and the acquisition of the write guard.

You should really just acquire a single write guard in the first place, and use the Entry API.

[H2CO3]

There are other problems with your code, too. The formatting, the use of whitespace, and the naming of fields are all non-idiomatic. returns are superfluous (Rust is an expression languauge).

Here's a correct, more idiomatic, and less verbose rewrite:

struct Table<'a> {
    name: &'a str,
}

impl<'a> Table<'a> {
    fn new(name: &'a str) -> Self {
        Self { name }
    }

    fn name(&self) -> &str {
        self.name
    }
}

#[derive(Default)]
struct TableManager<'a> {
    tables: RwLock<HashMap<&'a str, Arc<Table<'a>>>>,
}

impl<'a> TableManager<'a> {
    fn new() -> Self {
        Self::default()
    }

    fn open(&self, name: &'a str) -> Arc<Table> {
        self.tables
            .write()
            .unwrap()
            .entry(name)
            .or_insert_with(|| Arc::new(Table::new(name)))
            .clone()
    }
}

[bllhastings]

Sincerely appreciated.

Thanks.