From 4e6bece08aea01859a232e99a1e1ad8cc1eb7d36 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Wed, 9 Jun 2010 14:01:51 +0200 Subject: HSCondition: Fixed terrible bug which it inherited from its default python Condition implementation, related to the notify method not being treadsafe. Although I was aware of it, I missed the first check which tests for the size - the result could be incorrect if the whole method wasn't locked. Testing runs stable now, allowing to move on \! --- lib/git/async/util.py | 47 +++++++++++++++-------------------------------- 1 file changed, 15 insertions(+), 32 deletions(-) (limited to 'lib/git/async/util.py') diff --git a/lib/git/async/util.py b/lib/git/async/util.py index 008e60a3..2f46d55f 100644 --- a/lib/git/async/util.py +++ b/lib/git/async/util.py @@ -133,45 +133,28 @@ class HSCondition(deque): # END assure release lock def notify(self, n=1): - """Its vital that this method is threadsafe - to be fast we don'd get a lock, - but instead rely on pseudo-atomic operations that come with the GIL. - Hence we use pop in the n=1 case to be truly atomic. - In the multi-notify case, we acquire a lock just for safety, as otherwise - we might pop too much of someone else notifies n waiters as well, which - would in the worst case lead to double-releases of locks.""" - if not self: - return - if n == 1: - # so here we assume this is thead-safe ! It wouldn't be in any other - # language, but python it is. - # But ... its two objects here - first the popleft, then the relasecall. - # If the timing is really really bad, and that happens if you let it - # run often enough ( its a matter of statistics ), this will fail, - # which is why we lock it. - # And yes, this causes some slow down, as single notifications happen - # alot - self._lock.acquire() - try: + """Its vital that this method is threadsafe - we absolutely have to + get a lock at the beginning of this method to be sure we get the + correct amount of waiters back. If we bail out, although a waiter + is about to be added, it will miss its wakeup notification, and block + forever (possibly)""" + self._lock.acquire() + try: + if not self: # len(self) == 0, but this should be faster + return + if n == 1: try: self.popleft().release() except IndexError: pass - finally: - self._lock.release() - # END assure lock is released - else: - self._lock.acquire() - # once the waiter resumes, he will want to acquire the lock - # and waits again, but only until we are done, which is important - # to do that in a thread-safe fashion - try: + else: for i in range(min(n, len(self))): self.popleft().release() # END for each waiter to resume - finally: - self._lock.release() - # END assure we release our lock - # END handle n = 1 case faster + # END handle n = 1 case faster + finally: + self._lock.release() + # END assure lock is released def notify_all(self): self.notify(len(self)) -- cgit v1.2.1