kvdb-rocksdb: fix deadlock caused by double read lock

[BurtonQin]

This PR fixes a deadlock caused by double read lock in kvdb-rocksdb/src/lib.rs.

The first read lock is in write():

parity-common/kvdb-rocksdb/src/lib.rs

Lines 480 to 481 in 8e12159

	pub fn write(&self, tr: DBTransaction) -> io::Result<()> {
	match *self.db.read() {

Then iter_with_prefix() is called.

parity-common/kvdb-rocksdb/src/lib.rs

Line 511 in 8e12159

for (key, _) in self.iter_with_prefix(col, prefix) {

The second readlock is in iter_with_prefix():

parity-common/kvdb-rocksdb/src/lib.rs

Lines 575 to 576 in 8e12159

	fn iter_with_prefix<'a>(&'a self, col: u32, prefix: &'a [u8]) -> impl Iterator<Item = iter::KeyValuePair> + 'a {
	let read_lock = self.db.read();

According to parking_lot::RwLock:
“readers trying to acquire the lock will block even if the lock is unlocked when there are writers waiting to acquire the lock.”
“attempts to recursively acquire a read lock within a single thread may result in a deadlock.”

Therefore, when two read locks are interleaved by a write lock (e.g. in close(), restore()) from another thread, a deadlock happens.

The fix is to use a read_recursive() in iter_with_prefix(). It concerns me that read_recursive() may starve the write lock. But this seems the only solution currently.
Or maybe we can create a new version iter_with_prefix_recursive() using read_recursive() and use this version in write()

[parity-cla-bot]

It looks like @BurtonQin signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

[ordian]

Thanks for analysis! IMHO a cleaner approach would be to pass a read lock as an argument to iter_with_prefix.

[BurtonQin]

To maintain the original code logic,
RwLock::read_recursive() is unavoidable.
If we move the RwLockReadGuard created at L481 into the new_with_prefix() at L511,
then the following code will not be protected by the read lock.
It is also infeasible to pass a reference to the RwLockReadGuard as a parameter,
in that new_with_prefix() at L583 takes the RwLockReadGuard by move.
Therefore, We must create a new RwLockReadGuard other than the one created at L481.
So I think recursive seems the only choice.

[ordian]

To maintain the original code logic,
RwLock::read_recursive() is unavoidable.
If we move the RwLockReadGuard created at L481 into the new_with_prefix() at L511,
then the following code will not be protected by the read lock.
It is also infeasible to pass a reference to the RwLockReadGuard as a parameter,
in that new_with_prefix() at L583 takes the RwLockReadGuard by move.
Therefore, We must create a new RwLockReadGuard other than the one created at L481.
So I think recursive seems the only choice.

Good point. I've opened #397 as an alternative to this approach.

[ordian]

Closing in favor of #397