Issue8800
Created on 2010-05-24 09:55 by krisvale, last changed 2010-05-24 20:44 by jyasskin.
| Files | ||||
|---|---|---|---|---|
| File name | Uploaded | Description | Edit | |
| rwlock.patch | krisvale, 2010-05-24 09:55 | review | ||
| Messages (6) | |||
|---|---|---|---|
| msg106350 - (view) | Author: Kristján Valur Jónsson (krisvale) * | Date: 2010-05-24 09:55 | |
Provided is a patch, implementing a RWLock using a ConditionVariable and Lock. It has rdlock() and wrlock() methods, and an unlock() method, similar to the rwlock API in pthreads. It has context managers rdlocked() and wrlocked(). In addition, it conforms to the RLock interface, with acquire() and release() being aliases to wrlock() and unlock() respectively and when used as a context manager, it gets a wrlock() There is no documentation yet, since this is just a proposal, but there are unit tests. See also issue 8777 for another locking primitive, the Barrier. |
|||
| msg106372 - (view) | Author: Jeffrey Yasskin (jyasskin) * ![]() |
Date: 2010-05-24 16:39 | |
This patch doesn't apply cleanly against the py3k tree. Since Python 2.7 is in beta, and there's no 2.8, this can only go into python 3, so you should work against that tree.
It's a bit annoying that the R in RWLock stands for a different word from in RLock. But I can't think of a better name.
Hm, the test for RWLock should use a Barrier to check for multiple readers.
I wonder if it makes sense to add a DeadlockError as a subclass of RuntimeError and throw that from trying to upgrade a read-lock to a write-lock.
The docs should describe why upgrading a read-lock to a write-lock is banned, or else people will try to add the feature.
test_wrrecursion should also check pure writer recursion. Oh, no, that's tested by RLockTests. Comment that please. The name of the test should probably mention write_then_read_recursion to distinguish it from write-then-write or read-then-write.
test_readers_writers doesn't quite test enough. (Feel free to add another test rather than changing this one.) You want to test that readers can't starve writers. For example, say we have:
lock = RWLock()
done = False
def r():
with lock.rdlocked():
if done:
return
_wait()
def w():
nonlocal done
with lock.wrlocked():
done = True
readers = Bunch(r, 50)
_wait()
writers = Bunch(w, 1)
readers.wait_for_finished()
writers.wait_for_finished()
In a naive implementation, there may never be a point where no reader has the lock held, and so the writer may never get in to tell them all to exit. I believe your implementation will already pass this test.
spelling: mathced
I'm not sure that "#threads will be few" is true for a read-lock, but I don't mind for the first implementation.
Generally put a space after # when it introduces a comment, start comments with a capital letter, and end them with punctuation so we know you didn't stop typing early.
In _rdlock could you flip the "not self.nw" condition? That way the else won't be a double-negative.
Can self.rcond.wait() ever throw an exception? I suspect you should use try:finally: to guard the "self.nr -= 1"
Why must the user hold a write lock to use Condition.wait? Semantically, a waiting on a read-lock would release the current thread's recursive read-locks, and all should be well.
It looks like self.owning holds one copy of each thread's id for each re-entry by that thread. That wasn't obvious so deserves a comment.
General API questions:
1. Given that the other locking APIs use "acquire" and "release" instead of "lock" and "unlock", perhaps this class should use "rdacquire" and "wracquire"?
2. I wonder if it helps actual code to be able to just call "unlock" instead of "rdunlock" and "wrunlock". Code releasing the lock should always know which kind of lock it holds.
Otherwise, this looks basically correct. Thanks!
|
|||
| msg106373 - (view) | Author: Antoine Pitrou (pitrou) * ![]() |
Date: 2010-05-24 16:40 | |
I'm not sure it's a good idea to have a default context manager, and default acquire/release methods. "In the face of ambiguity, refuse the temptation to guess". Is there any use in being able to use a RWLock inside a condition variable? Docstrings should be polished (e.g. typo "must be mathced with an release"). I think wrlocked() and rdlocked() deserve a docstring too. A style nit: generally, comments should be on their own line, before the code they comment. Also, there should be a space just after the "#" (see PEP 8 :-)). |
|||
| msg106376 - (view) | Author: Jeffrey Yasskin (jyasskin) * ![]() |
Date: 2010-05-24 17:43 | |
In this case, "acquire" isn't ambiguous. All the other lock types actually acquire a write lock, so it makes sense to have the operation with the same name they use also acquire a write lock on this object. I realized that read/write locks are actually shared/exclusive locks, which might form the basis for a name that doesn't collide with RLock. Boost (http://www.boost.org/doc/libs/1_43_0/doc/html/thread/synchronization.html#thread.synchronization.mutex_types.shared_mutex) uses shared_mutex for the concept, so SLock or SELock? There are some algorithms that write while the lock is acquired non-exclusive, so "shared" is actually a better name for the concept, even though posix and Java used read/write. The possibility of lock downgrading (turning an exclusive lock into a shared lock, without allowing any other exclusive acquisitions in the mean time) might inform your decision about how to name "unlock". |
|||
| msg106377 - (view) | Author: Antoine Pitrou (pitrou) * ![]() |
Date: 2010-05-24 17:51 | |
> In this case, "acquire" isn't ambiguous. All the other lock types > actually acquire a write lock, so it makes sense to have the operation > with the same name they use also acquire a write lock on this object. Well, it looks like a strange kind of "consistency" to me, since other lock types are not dual. Both posix and Java APIs don't seem to imply that the write lock is the "default" lock in a RW lock. However, I'm not a synchronization specialist. > I realized that read/write locks are actually shared/exclusive locks, > which might form the basis for a name that doesn't collide with RLock. > Boost > (http://www.boost.org/doc/libs/1_43_0/doc/html/thread/synchronization.html#thread.synchronization.mutex_types.shared_mutex) uses shared_mutex for the concept, so SLock or SELock? SELock looks ok, but I have to say that RWLock is more obvious (I don't need to do a search to guess that RW means "read/write"). |
|||
| msg106386 - (view) | Author: Jeffrey Yasskin (jyasskin) * ![]() |
Date: 2010-05-24 20:44 | |
You're definitely right that posix and java don't make these usable from the normal lock API. And I don't think it's essential that they be usable as RLocks, although it's nice for Conditions. I think what I'm actually saying is that, if you have an acquire() operation on RWLock, then it has to forward to the write lock. Forwarding to the read lock would have different semantics from RLock.acquire (2 threads could get in at once) and so would be incorrect. I agree that RWLock is more obvious. As long as the docs mention that it's equivalent to a shared/exclusive lock, I don't care that much about the name. Just throwing out ideas. |
|||
| History | |||
|---|---|---|---|
| Date | User | Action | Args |
| 2010-05-24 20:44:35 | jyasskin | set | keywords:
patch, patch, needs review messages: + msg106386 |
| 2010-05-24 17:51:11 | pitrou | set | messages: + msg106377 |
| 2010-05-24 17:43:38 | jyasskin | set | keywords:
patch, patch, needs review messages: + msg106376 |
| 2010-05-24 16:40:05 | pitrou | set | keywords:
patch, patch, needs review nosy: + pitrou messages: + msg106373 |
| 2010-05-24 16:39:11 | jyasskin | set | keywords:
patch, patch, needs review messages: + msg106372 |
| 2010-05-24 11:41:07 | pitrou | set | keywords:
patch, patch, needs review nosy: + jyasskin stage: patch review versions: + Python 3.2, - Python 2.7 |
| 2010-05-24 09:55:40 | krisvale | create | |
