dashmap
dashmap copied to clipboard
xacrimon
Document deadlock conditions
Hi @xacrimon. I was looking at the documentation of dashmap, and the documentation of methods that access the map warn about potential deadlocks. For mutable methods it says
**Locking behaviour:** May deadlock if called when holding any sort of reference into the map.
With no further knowledge about the implementation details this seems that any concurrent use of these methods might can cause deadlocks. The pessimistic interpretation of this is that you can't ever use these methods in a concurrent program.
Of course in practice I imagine this is not a problem and if all thread "follow the rules" no deadlock will happen. I imagine the rule is something like: if all thread eventually drop the borrowed references to the dashmap the system is guaranteed to progress (unless the same thread tries to borrow data from the dashmap more than once at the same time).
It would be good to document the precise conditions that can lead to deadlock, so users can be sure their program is not going to deadlock when accessing the map.
I would agree with this; I have encountered deadlocks using dash map though my use case is a bit different. It would be great to understand the conditions.
Somewhere at the top of Lib.rs it mentions:
/// Documentation mentioning locking behaviour acts in the reference frame of the calling thread.
/// This means that it is safe to ignore it across multiple threads.
So i am assuming it should be read as (in poor English 😄):
**Locking behavior:** May deadlock if called when **the current thread is** holding any sort of reference into the map.
Yes, I'll update this. Thanks folks. The interpretation by @timvw01 is correct. This happens because referencing an entry requires getting a lock on a pseudorandom rwlock and such you cannot violate the rwlock principles.
So I guess DashMap isn't very suitable to use in an async runtime like Tokio in which even if the async operations are supposedly running in a multi-threaded async environment isn't guaranteed to run tasks on different threads and therefore holding a ref to the map across awaits might also deadlock?
@Nader-Sl Correct, holding refs across awaits is dangerous. But really, you probably shouldn't hold refs for long periods anyway as it might delay other tasks.
@Nader-Sl Correct, holding refs across awaits is dangerous. But really, you probably shouldn't hold refs for long periods anyway as it might delay other tasks.
Agreed :)
DashMap isn't very suitable to use in an async runtime like Tokio
It would be nice to write this in the readme at least people are aware of it, wouldn't it?
DashMap isn't very suitable to use in an async runtime like Tokio
It would be nice to write this in the readme at least people are aware of it, wouldn't it?
I would argue here, it is suitable to use in async runtimes, but you cannot hold refs across awaits. IMHO it is reasonable requirement.
Thanks @DariuszOstolski, do you mean that as long as you don't hold refs across await points it is guaranteed that there are no deadlocks? I'd like to understand more about how this is ensured, are there any useful resources that can help (besides the source code)?
Thanks @DariuszOstolski, do you mean that as long as you don't hold refs across await points it is guaranteed that there are no deadlocks? I'd like to understand more about how this is ensured, are there any useful resources that can help (besides the source code)?
No. Deadlocks could occur even without crossing await points. For example, attempting to remove an entry, while holding a reference to the underlying value would result in a deadlock. But, apart from that, it is generally a bad idea to lock data across await points, because another task might need access to the data stored behind the lock.
Is there any efforts in adding a dashmap with a tokio::sync::RwLock to prevent these issues? even tho is bad practice to hold locks for long periods it would be better to realize the problem on a system running slower rather than completely blocking, maybe adding a alternative AsyncSafeDashMap, or there are problems on trying to design a dashmap this way?
