Add RwLock

[cjayross]

Resolves #2015

This branch adds an asynchronous version of std::sync::RwLock to futures_util::lock.

New public data types:

• futures::lock::RwLock
• futures::lock::RwLockReadFuture
• futures::lock::RwLockWriteFuture
• futures::lock::RwLockReadGuard
• futures::lock::RwLockWriteGuard

Feel free to check over the additions. Though I mostly based the code off of the implementation of futures::lock::Mutex and async_std::sync::RwLock

[cjayross]

At the moment, there is a deadlocking problem when multiple requests for write access are made.

[parasyte]

With this implementation (and the async-std implementation on which it is based), read-heavy workloads will starve writers.

Starvation occurs here:

futures-rs/futures-util/src/lock/rwlock.rs

Lines 101 to 106 in 1430b05

	match self.state.compare_exchange_weak(
	state,
	state + ONE_READER,
	Ordering::SeqCst,
	Ordering::SeqCst,
	) {

by incrementing the reader count without regard for the number of writers waiting to acquire the lock.

See also:

• https://docs.rs/parking_lot/0.10.0/parking_lot/type.RwLock.html
• RwLock needs a task-fair locking policy asomers/futures-locks#34

[cjayross]

@parasyte The function RwLock::try_read is only responsible for creating a guard should the state of the lock allow it. We could change the priority between read/write access by controlling how waiters are awoken when a guard is dropped.

[cjayross]

@parasyte At the moment what I have is this:

futures-rs/futures-util/src/lock/rwlock.rs

Lines 417 to 431 in 1430b05

	match (old_state & HAS_WAITERS, old_state & READ_COUNT_MASK) {
	(0, 0) => {}
	(0, _) => {
	let mut readers = self.rwlock.read_waiters.lock().unwrap();
	for (_, waiter) in readers.iter_mut() {
	waiter.wake();
	}
	}
	_ => {
	let mut writers = self.rwlock.write_waiters.lock().unwrap();
	if let Some((_, waiter)) = writers.iter_mut().next() {
	waiter.wake();
	}
	}
	}

Which only wakes waiting readers if there are no currently waiting writers.
Though, this is flawed at the moment since the read counter isn't counting the number of waiting readers, it counts the number of active readers—which in this case is always going to be 0.

[parasyte]

I'm confident the issue is in try_read because any task that awaits on the RwLockReadFuture guard will keep the read state alive without allowing writers a chance to acquire; try_write will never succeed as long as there is at least one reader holding the lock.

A second reader can come along at any time and also acquire the lock because try_read will allow the lock to be acquired regardless of how many writers are waiting. Even if the first reader decides to release its lock after the second has acquired, this will leave just one reader task that is newer (waits less time) than the writer task.

This is where read-heavy workloads become a problem. A writer will be blocked forever.

[cjayross]

I see your point; so what you're arguing is that we establish more complex rules for acquiring read locks? I think that's doable and could work out fine.
Perhaps something like this:

• If a writer becomes blocked, no new readers are allowed to acquire the lock.
• Once a write guard is dropped, wake a waiting writer; if there are no waiting writers, wake all readers.

Although, from the sound of it, it seems like we will then be starving readers in write-heavy applications.

[parasyte]

The docs I pointed to in parking_lot describe a similar fairness policy. The first property of fairness discussed is that readers should not acquire the lock if there is a writer waiting.

This lock uses a task-fair locking policy which avoids both reader and writer starvation. This means that readers trying to acquire the lock will block even if the lock is unlocked when there are writers waiting to acquire the lock. Because of this, attempts to recursively acquire a read lock within a single thread may result in a deadlock.

The second property is termed "eventual fairness", which is maybe closer to what you have been trying to address? They describe a situation where a single task may starve other tasks by repeatedly locking and unlocking without yielding to other tasks. In the parking_lot implementation, unlock fairness is forced periodically, which allows the lock to go to the next thread by queue order.

In other words, fairness is based on the order in which tasks attempt to acquire the lock: https://github.com/Amanieu/parking_lot/blob/fa294cd677936bf365afa0497039953b10c722f5/src/raw_rwlock.rs#L21-L32

// This reader-writer lock implementation is based on Boost's upgrade_mutex:
// https://github.com/boostorg/thread/blob/fc08c1fe2840baeeee143440fba31ef9e9a813c8/include/boost/thread/v2/shared_mutex.hpp#L432
//
// This implementation uses 2 wait queues, one at key [addr] and one at key
// [addr + 1]. The primary queue is used for all new waiting threads, and the
// secondary queue is used by the thread which has acquired WRITER_BIT but is
// waiting for the remaining readers to exit the lock.
//
// This implementation is fair between readers and writers since it uses the
// order in which threads first started queuing to alternate between read phases
// and write phases. In particular is it not vulnerable to write starvation
// since readers will block if there is a pending writer.

Comparing to parking_lot::RwLock is not really ideal, since this implementation is quite complex and handles a lot of edge cases. (It even supports upgrading and downgrading guards. cf. async-rs/async-std#704)

[cjayross]

Nonetheless, I think it would suffice for now to allow the pull request to merge once it is deemed suitable. Dealing with fairness policies seems beyond the scope of this pull request and should be left to a dedicated issue.

[najamelan]

It's just an opinion, but I wouldn't merge a feature with known issues. Might as well make it as correct as possible before merging. Since it's async, I would assume that you have a queue of things waiting to obtain the lock. Once a writer is in first place, you wait until everything else releases the lock and give it to them.

You can make a queue system where it is FIFO, but when a reader asks for the lock and there is already a reader in the queue, just bundle them all together and wake them all together. After you wake them, if a reader comes in and the queue is empty, they can immediately join the party, but if a writer comes in, that goes in first place and further readers coming in all get grouped in the next slot. That way I don't think anything can starve.

That's just a really basic design of the top of my head, just to say I don't think it's an insurmountable problem. But I'm sure people have written white papers and thesises about how you can do this in really smart ways, and if all else fails, tokio seems to have an impl that is fair, based on a semaphore, so that can provide inspiration as well.

[cjayross]

@najamelan The point I was trying to make is that this pull request is focused on the introduction of new data types that provide asynchronous read-write locks. The improvement and optimization of these locks is, in my opinion, a separate matter.

If there is an issue with the code itself such that this request at the moment would fail to provide correct read-write locks, then that is something to fix before merging. Otherwise, it would be more efficient to merge just a basic implementation then improve it in separate branches.

[cjayross]

So I've managed to get an implementation together that can support fairness for Read/Write locks. However, I'm currently blocked. I believe that I will need to create custom waker functions, and while I considered using ArcWake for this, I don't know if it would be wise to require types that are stored in these locks to be thread-safe.

@taiki-e @cramertj Would it be possible to add additional helper traits/methods in futures-task for creating waker functions that don't require Arc?

[taiki-e]

Would it be possible to add additional helper traits/methods in futures-task for creating waker functions that don't require Arc?

Since Waker is Send + Sync, I don't think we can add safe helper traits/methods for !Send or !Sync wakers.

[cjayross]

Alright, I'll see if I can make some adjustments. To clarify though, am I correct in avoiding the additional Sync restraint?

Edit: Nevermind, the route I was going with was nonsense.

[cjayross]

So, what I have is what I think should produce a fair read/write lock. The way it works uses a ticketing system. Each request for a lock takes a ticket that determines under what circumstances the lock can be granted.

The way that it works, and how it manages fairness, is by allocating locks in an alternating style between readers and writers. If there are no write requests, readers are provided locks upon each request; however, when a write request does come along, all read requests that come after are put on hold until the next read phase. Once the current read phase completes, the write request is given the lock, marking the start of a write phase. Once the write phase completes, the next read phase begins and the cycle repeats from there.

However, the solution I have here is pretty airtight; and by airtight, I mean its exceptionally inflexible. As of now, I don't really know how to alter it to allow for methods such as try_read or try_write because each request is responsible for maintaining a given ticket and updating state appropriately when it becomes the request's "turn". If an event broadcasts and no request is available to say "oh, I'm up!" then the whole system deadlocks. I don't like that this at all. Though, other than that, the solution is pretty elegant. Overall, the system operates with very little state and very little overhead.

[cjayross]

@taiki-e Could you provide any help setting up a test environment. I'm unfamiliar with the general procedure for adding a new unstable feature to a package like this.

[cjayross]

Alright, from what I managed to see, this should work and provide fair read/write locks. I managed to add in support for try_read and try_write without resorting to timers as I have seen from other implementations. I also have a few tests, but they haven't been organized to you guys' standards.

Let me know when you guys are available so we can get this going.

@cramertj and @taiki-e, I'll need for someone to review the code and to help integrate this as an unstable feature.
@parasyte I'd also be curious on what your thoughts are on my implementation.

[notgull]

If we use the spin crate, and replace the AtomicUsize/UnsafeCell pair with a spin::RwLock we may deduplicate a lot of code.

[notgull]

IIRC This is generally implemented using another mutex instead of just another water list, but I'm not aure

[notgull]

If you use the spin types here this should be easier to implement