Author jyasskin
Recipients jyasskin, kristjan.jonsson
Date 2010-05-24.16:39:09
SpamBayes Score 1.5732e-05
Marked as misclassified No
Message-id <1274719152.87.0.271850521769.issue8800@psf.upfronthosting.co.za>
In-reply-to
Content
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!
History
Date User Action Args
2010-05-24 16:39:12jyasskinsetrecipients: + jyasskin, kristjan.jonsson
2010-05-24 16:39:12jyasskinsetmessageid: <1274719152.87.0.271850521769.issue8800@psf.upfronthosting.co.za>
2010-05-24 16:39:11jyasskinlinkissue8800 messages
2010-05-24 16:39:09jyasskincreate