RwLock: Chaining read and write?

[abitofhelp]

Hello,

I am wondering whether there is a better way to determine whether a shared vector contains an object and if it does not exist, push the new object into the vector. The first section of code, below, works as expected. The second set of code, which is commented out, deadlocks at the push().

I'd appreciate your suggestions for writing the best version of this code. It will be used in a Rust microservice using Tokio, etc., when things are correct.

The full project can be found at: abitofhelp.shared / rwlocking · GitLab

pub struct DatabaseConnection<T> {
    pub database: T,
}

pub type InMemoryDatabase = RwLock<Vec<ProjectEntity>>;

pub type InMemoryDatabaseConnection = Arc<DatabaseConnection<InMemoryDatabase>>;

pub fn new() -> InMemoryDatabaseConnection {
    let mut inmemory_database = RwLock::new(vec![
        ProjectEntity::new(None, "abc".to_string(), 123, "America_Phoenix".to_string()).unwrap(),
        ProjectEntity::new(None, "xyz".to_string(), 321, "America_Chicago".to_string()).unwrap(),
    ]);

    InMemoryDatabaseConnection::from(DatabaseConnection {
        database: inmemory_database,
    })
}

fn main() -> std::io::Result<()> {
    let database_connection = inmemory_database_connection::new();

    let project = Project::from("zzz", 010, "UTC");
    create_project(database_connection.clone(), project).map_err(|e| panic!("{e}"))
}

fn create_project(conn: InMemoryDatabaseConnection, project: Project) -> Result<(), ApiError> {

    let project_name = project.name.clone();

    {
        println!(
            "Searching for an existing project named '{}'",
            &project.name
        );

        {
            let read_binding = conn.database.read();
            let exists = read_binding.iter().find(|p| p.name == project_name);
            if exists.is_some() {
                return Err(ApiError::NotFound {
                    message: format!("a project named '{}' already exists", &project_name),
                    err: None,
                });
            }
        }

        {
            println!("A project named '{}' does not exist", &project_name);
            let project_entity = ProjectEntityMapper::to_entity(project);
            println!(
                "Attempting to create() the project named '{}'",
                &project_name
            );
            let mut write_binding = conn.database.write();
            let result = write_binding.push(project_entity);
            println!("Created the project named '{}'", &project_name);
            Ok(result)
        }
    }
}

// IS IT POSSIBLE TO CHAIN THE READ AND WRITE?  BASED ON MY READING AND EXPERIMENTING, IT LOOKS LIKE THE FOLLOWING IS NOT FEASIBLE.
//
// conn.database
//     .read().iter()
//     .find(|p| p.name == project.name)
//     .map_or_else(
//         || {
//             println!(
//                 "A project named '{}' does not exist -- proceeding with create()",
//                 &project.name
//             );
//             let project_entity = ProjectEntityMapper::to_entity(project);
//             println!("Attempting to create() the project");
//             conn.database.write().push(project_entity);
//
//             Ok(())
//         },
//         |pe| {
//             Err(ApiError::NotFound {
//                 message: format!("a project named '{}' already exists", pe.name),
//                 err: None,
//             })
//         },
//     )
// }

[tristan]

You might need to be a bit more specific about what problem you're trying to solve by "chaining" a read() into a write(). I don't think your commented out code, even if we imagine that it could somehow drop the read lock inside the map_or_else closure, does anything different practically. If you just want to refactor it because it feels nicer to have a functional style, I'd say move just move on .

Also, side note: instead of the iter().find(...).is_some() combo on lines 32/33, you can use iter().any(...) to get a boolean directly.

[drewtato]

Edit: don't use this, read my reply

How about this

if let Some(pe) = conn.database.read().iter().find(|p| p.name == project.name) {
    return Err(ApiError::NotFound {
        message: format!("a project named '{}' already exists", pe.name),
        err: None,
    });
}

println!(
    "A project named '{}' does not exist -- proceeding with create()",
    &project.name
);
let project_entity = ProjectEntityMapper::to_entity(project);
println!("Attempting to create() the project");

conn.database.write().push(project_entity);
Ok(())

You just need to ensure the read guard is dropped before trying to obtain the write guard. This happens at the end of the if let block. One thing that wouldn't work is putting the rest of this in an else block, since the guard wouldn't be dropped by then.

[khimru]

What's the high-level goal? How would you algorithm work?

High-level overview:

Time	Thread A	Thread B
Step1	Gets read lock	Gets read lock
Step2	Finds out object "X" doesn't exist	Finds out object "X" doesn't exist
Step3	Creates object "X"	Creates object "X"
Step4	What happens here now?	What happens here now?

The problem with RwLock is the same as with &mut: it's not, really, read+write lock. but more of shared+exclusive lock.

When you take the read lock you can do certain things, but other tasks can do the exact same thing! And when you try to “switch”… makes no sense.

Same as with trying to go from & to &mut in Rustonomicon:

• Transmuting an & to &mut is always Undefined Behavior.
• No you can't do it.
• No you're not special.

P.S. You probably want double-checked lock, but that's different from naïve attempts to “upgrade” which, of course, make no sense whatsoever to a degree that Rustonomicon needs to reject them thrice.

[drewtato]

I realized later that this is no good. There's a few options left:

• If you can allow the "project already exists" branch to require a write lock (for example, if this is already checked once by the client, or if project creation is rare), then it is simple and straightforward to acquire a write lock at the beginning and use that throughout.
• If you can't allow the "project already exists" branch to require a write lock every time (for example, if this will occur often), then you can still check the first time with a read lock, but if you find it does not exist, then you need to check again once you acquire the write lock, since something could have added a project with the same name between the two locks.
• If you can't allow the "project already exists" branch to require a write lock ever (perhaps for DOS prevention?) then you'll need a different synchronization type that lets you upgrade lock guards from read to write, or have another lock to ensure nothing is modified in between reading and writing.

[samuelpilz]

parking_lot has upgradable reads. The docs suffer from abstractionitis and are only useful for experts of that crates (which I am not, it always takes me ages to find the correct functions). The read_upgradable function can be found in the loc_api crate.

example: rustexplorer (nice api-design is left as exercise to the reader)

fn lock_get_or_insert_one(lock: &RwLock<Option<u8>>) -> RwLockReadGuard<'_, Option<u8>> {
    let mut guard = lock.upgradable_read();
    if guard.is_none() {
        guard.with_upgraded(|x| {
            *x = Some(1);
        });
    }
    RwLockUpgradableReadGuard::downgrade(guard)
}

[samuelpilz]

Note that, upgradable_read is exclusive with other upgradable reads (but allows other nonupgradable reads). If your exists-check is expensive and dominates the computation, the upgrade-approach might be equivalent to a Mutex.
If re-checking is not an option (readlock-check-writelock-check-insert), then exclusive is necessary.

Note that an upgrade from a standard read-lock leads to almost guaranteed deadlocks. Consider the sequence: Thread1 read-locks, Thread2 read-locks, Thread1 wants upgrade, waits for all read-locks to expire, Thread2 wants upgrade, waits for all read-locks to expire. While detecting such cycles is possible, it is expensive not supported by parking_lot and lock_api.

[kornel]

In these situations (assuming you read it a lot, but write only once) checking twice is the right pattern.

1. lock for reading, check if it's set, return on success
2. drop read lock
3. lock for writing
4. check if it's set, and write to it if it's not set
5. if using parking_lot locks, downgrade the write lock. If using std, drop write lock and goto 1.

[abitofhelp]

Thank you, everyone, for your replies and discussion. This service will handle many more reads than writes in a given period of time. Based on your comments and not using parking_lot's upgradeable locks, kornel's pattern is applicable. However, when I tried to implement the validation of acquiring the read or write lock, it reported that no lock was acquired (commented out code below). If I assume that try_read() and try_write() return successful locks, then the code works. Is there an error in my interpretation of your comments on this scenario? This is not production-quality code... It is just for sorting out issues...

I like the idea of using parking_lot's upgradeable read/write lock, but I need to read the docs about how to implement it.

I really appreciate your feedback and comments. Thanks, again for your help!

Mike

    pub fn create(&self, project: Project) -> Result<(), ApiError> {
        // Since the internal database id is not used to identify projects, we
        // need to validate that the project's name does not exist.

        if self.conn.database.try_read().is_some() {
            //if self.conn.database.is.is_locked() {
                if self
                    .conn
                    .database
                    .read()
                    .iter()
                    .find(|p| p.name == project.name.to_string())
                    .is_some()
                {
                    return Err(ApiError::Conflict {
                        message: format!("a project named '{}' already exists", project.name),
                        err: None,
                    });
                }
            // } else {
            //     return Err(ApiError::Application {
            //         message: format!("failed to acquire the read-lock"),
            //         err: None,
            //     });
            // }
        } else {
            return Err(ApiError::Application {
                message: format!("failed to acquire the read-lock"),
                err: None,
            });
        }

        if self.conn.database.try_write().is_some() {
            //if self.conn.database.is_locked() {
                let project_entity = ProjectEntityMapper::to_entity(project);
                return Ok(self.conn.database.write().push(project_entity));
            } else {
                return Err(ApiError::Application {
                    message: format!("failed to acquire the write-lock"),
                    err: None,
                });
            }
        // } else {
        //     return Err(ApiError::Application {
        //         message: format!("failed to acquire the write-lock"),
        //         err: None,
        //     });
        // }
    }
}

[moy2010]

@abitofhelp

However, when I tried to implement the validation of acquiring the read or write lock, it reported that no lock was acquired (commented out code below)

Parkinglot's RWLock try_ methods will only return Some if the lock was successfully acquired. You can read the documentation.

Just go ahead and clean your code now that you have cleared the deadlock.

[kornel]

try_ methods give up immediately, and should be treated as unreliable. Use them for opportunistic tricks when you implement multiple different locking strategies, or don't care if you ever lock the lock.
Otherwise use the real read()/write() that will reliability obtain a lock.

[drewtato]

@abitofhelp

This service will handle many more reads than writes in a given period of time.

Yeah but this method is specifically for writing data. The important thing to consider isn't the frequency of this method being called, this is about the frequency of trying to write a duplicate project name. If many duplicates are expected, then reading and writing might make sense. If duplicates are rare, then you might as well get a write lock no matter what.

[tristan]

@abitofhelp

Is there an error in my interpretation of your comments on this scenario?

Yes, I believe you've misunderstood kornel's comment. What they've outlined is exactly what your original code was already doing. If you're just looking for a less "messy" looking implementation, others have given good suggestions already, and drewtato's suggestion of only using a write lock if duplicates are expected to be rare is good advice as this would eliminate any possibility of race conditions with 2 threads calling create with the same project name at the same time.

e.g.

fn create_project(conn: InMemoryDatabaseConnection, project: Project) -> Result<(), ApiError> {
    let mut lock = conn.database.write();
    if lock.iter().any(|p| p.name == project.name) {
        return Err(ApiError::NotFound {
            message: format!("a project named '{}' already exists", &project.name),
            err: None,
        });
    }
    let project_entity = ProjectEntityMapper::to_entity(project);
    lock.push(project_entity);
    Ok(())
}

Unless you're calling create_project 1000s of times per second, or your ProjectEntityMapper::to_entity function takes a long time (e.g. 100s of milliseconds), you probably wont notice any performance issues with using only a write lock, and if you find that it becomes a problem later on, you can look into optimizing it then.