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

feat req: try_insert #245

Open
gitmalong opened this issue Jan 1, 2023 · 2 comments
Open

feat req: try_insert #245

gitmalong opened this issue Jan 1, 2023 · 2 comments

Comments

@gitmalong
Copy link

gitmalong commented Jan 1, 2023

When debugging deadlocks one may want to get more insights and check if a key is locked before doing an insert. Currently this can be done with

match map.try_get_mut(&key) {
    dashmap::try_result::TryResult::Present(mut v) => v.0 = value.0,
    dashmap::try_result::TryResult::Absent => {
        liq_pool_store.insert(key, value); // Deadlock can still occur as another thread could have acquired a ref
    }
    dashmap::try_result::TryResult::Locked => error!("Skipped insert due to lock, key {}", &key),
};

A nicer approach with less boilerplate and no-deadlock guarantee could be:

match map.try_insert(key.clone(), value) {
    dashmap::try_result::TryInsertResult::Inserted => info!("Inserted value for key {}", key),
    dashmap::try_result::TryInsertResult::Locked => error!("Skipped insert due to lock, key {}", &key),
}

Example use case:
When facing a deadlock and switching from insert(k,v) to try_insert(k,v) as shown above and no key is repeatedly printed as locked it becomes unreasonable why the dead lock occurs at all (if the remaining code in the block does not interact with the map). Is that assumption right?

@imagine-hussain
Copy link

imagine-hussain commented Mar 13, 2023

How would you consolidate this with the try_insert that already exists on HashMap that does:

Tries to insert a key-value pair into the map, and returns a mutable reference to the value in the entry.
If the map already had this key present, nothing is updated, and an error containing the occupied entry and the value is returned.

Alternatively, would using try_entry(&'a self, key: K) -> Option<Entry<'a, K, V, S>> work?
Since Entry itself contains a RWLockWriteGuard over the shard that the entry should be in, I think that should prevent deadlock right? Since there isn't a moment like in the first example where you don't have a lock over the map?

I'm thinking of something like

match map.try_entry(key) {
  None => error!("Skipped insert due to lock, key: {}", key),
  Some(entry) => {
    entry.and_modify(|e| e.0 = value.0).or_insert(value);
  }
}

@mscofield0
Copy link
Contributor

Please implement this

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

No branches or pull requests

3 participants