[Twisted-Python] Re: [Twisted-commits] Today is screw with twisted's guts day.
On Wed, Nov 19, 2003 at 02:07:39PM -0700, exarkun CVS wrote:
Index: Twisted/twisted/internet/base.py diff -u Twisted/twisted/internet/base.py:1.67 Twisted/twisted/internet/base.py:1.68 --- Twisted/twisted/internet/base.py:1.67 Fri Oct 24 12:45:58 2003 +++ Twisted/twisted/internet/base.py Wed Nov 19 14:07:38 2003 @@ -400,12 +400,12 @@ """Run all pending timed calls. """ if self.threadCallQueue: - for i in range(len(self.threadCallQueue)): - callable, args, kw = self.threadCallQueue.pop(0) + for (f, a, kw) in self.threadCallQueue: try: - callable(*args, **kw) + f(*a, **kw) except: - log.deferr() + log.err() + del self.threadCallQueue[:] now = time() while self._pendingTimedCalls and (self._pendingTimedCalls[-1].time <= now): call = self._pendingTimedCalls.pop()
I think this change introduces a race condition. It's possible for a thread to call callFromThread between the end of the for loop and the del statement. (callFromThread is defined as: def callFromThread(self, f, *args, **kw): """See twisted.internet.interfaces.IReactorThreads.callFromThread. """ assert callable(f), "%s is not callable" % f if threadable.isInIOThread(): self.callLater(0, f, *args, **kw) else: # lists are thread-safe in CPython, but not in Jython # this is probably a bug in Jython, but until fixed this code # won't work in Jython. self.threadCallQueue.append((f, args, kw)) self.wakeUp() ) So I think it's possible to lose thread calls. I *think* this patch will fix it, but more eyeballs are always welcome with threaded code: diff -u -r1.68 base.py --- twisted/internet/base.py 19 Nov 2003 21:07:38 -0000 1.68 +++ twisted/internet/base.py 20 Nov 2003 00:52:08 -0000 @@ -400,12 +400,14 @@ """Run all pending timed calls. """ if self.threadCallQueue: + count = 0 for (f, a, kw) in self.threadCallQueue: try: f(*a, **kw) except: log.err() - del self.threadCallQueue[:] + count += 1 + del self.threadCallQueue[:count] now = time() while self._pendingTimedCalls and (self._pendingTimedCalls[-1].time <= now): call = self._pendingTimedCalls.pop() -Andrew.
On Thu, Nov 20, 2003 at 11:53:19AM +1100, Andrew Bennetts wrote:
[snip]
So I think it's possible to lose thread calls. I *think* this patch will fix it, but more eyeballs are always welcome with threaded code:
Thanks for noticing this.
diff -u -r1.68 base.py --- twisted/internet/base.py 19 Nov 2003 21:07:38 -0000 1.68 +++ twisted/internet/base.py 20 Nov 2003 00:52:08 -0000 @@ -400,12 +400,14 @@ """Run all pending timed calls. """ if self.threadCallQueue: + count = 0 for (f, a, kw) in self.threadCallQueue: try: f(*a, **kw) except: log.err() - del self.threadCallQueue[:] + count += 1 + del self.threadCallQueue[:count] now = time() while self._pendingTimedCalls and (self._pendingTimedCalls[-1].time <= now): call = self._pendingTimedCalls.pop()
That looks a little more complex than it needs to be. What about this version? def runUntilCurrent(self): """Run all pending timed calls. """ if self.threadCallQueue: calls, self.threadCallQueue = self.threadCallQueue, [] for (f, a, kw) in calls: try: f(*a, **kw) except: log.err() I considered doing it this way first, but wasn't sure if anyone relied on the identity of threadCallQueue (not that anyone should, but one never knows). If anyone thinks changing its identity should be avoided, I'd like this version: def runUntilCurrent(self): """Run all pending timed calls. """ if self.threadCallQueue: calls = self.threadCallQueue[:] del self.threadCallQueue[:] for (f, a, kw) in calls: try: f(*a, **kw) except: log.err() I suppose reverting it entirely is an option, seeing as how no one has yet *actually* been bitten by the quadratic behavior (nor does it seem likely that anyone will be), but all other things equal, it's nicer to use O(N) algorithms deep down in the guts of Twisted than to use O(N**2) algorithms. Jp
On Wed, Nov 19, 2003 at 08:12:56PM -0500, Jp Calderone wrote:
On Thu, Nov 20, 2003 at 11:53:19AM +1100, Andrew Bennetts wrote: [patch to keep count of items used from the list]
That looks a little more complex than it needs to be. What about this version?
def runUntilCurrent(self): """Run all pending timed calls. """ if self.threadCallQueue: calls, self.threadCallQueue = self.threadCallQueue, [] for (f, a, kw) in calls: try: f(*a, **kw) except: log.err()
Yeah, that looks ok too, I think. I'm less certain though... is it possible for this to happen: reactor thread other thread -------------- ------------ call callFromThread ...get reference to self.threadCallQueue call runUntilCurrent ...set self.tCQ to a new list ...do for loop ...append to now old threadCallQueue So that again, a call can go missing? Hmm, I think this is possible, so that version is also bad. It's also harder to reason about than my patch -- I had to disassemble it to help me think about it :)
I considered doing it this way first, but wasn't sure if anyone relied on the identity of threadCallQueue (not that anyone should, but one never knows). If anyone thinks changing its identity should be avoided, I'd like this version:
def runUntilCurrent(self): """Run all pending timed calls. """ if self.threadCallQueue: calls = self.threadCallQueue[:] del self.threadCallQueue[:] for (f, a, kw) in calls: try: f(*a, **kw) except: log.err()
That's definitely racy! What if the call to callFromThread happens between the "calls = ..." and the del statement? A version which does calls = self.threadCallQueue[:] del self.threadCallQueue[:len(calls)] Should be ok though, for the same reason my patch is (or at least I think it is :) The main difference between that version and my is this one involves a small list copy, but it might still be faster than executing "count += 1" on every loop iteration. Either way, it's at the point where I don't care :)
I suppose reverting it entirely is an option, seeing as how no one has yet *actually* been bitten by the quadratic behavior (nor does it seem likely that anyone will be), but all other things equal, it's nicer to use O(N) algorithms deep down in the guts of Twisted than to use O(N**2) algorithms.
I think I'd like to see it improved to have O(N) complexity too. -Andrew.
On Thu, Nov 20, 2003 at 12:38:03PM +1100, Andrew Bennetts wrote:
On Wed, Nov 19, 2003 at 08:12:56PM -0500, Jp Calderone wrote:
On Thu, Nov 20, 2003 at 11:53:19AM +1100, Andrew Bennetts wrote: [patch to keep count of items used from the list]
That looks a little more complex than it needs to be. What about this version?
def runUntilCurrent(self): """Run all pending timed calls. """ if self.threadCallQueue: calls, self.threadCallQueue = self.threadCallQueue, [] for (f, a, kw) in calls: try: f(*a, **kw) except: log.err()
Yeah, that looks ok too, I think. I'm less certain though... is it possible for this to happen:
reactor thread other thread -------------- ------------ call callFromThread ...get reference to self.threadCallQueue call runUntilCurrent ...set self.tCQ to a new list ...do for loop ...append to now old threadCallQueue
So that again, a call can go missing? Hmm, I think this is possible, so that version is also bad. It's also harder to reason about than my patch -- I had to disassemble it to help me think about it :)
I considered doing it this way first, but wasn't sure if anyone relied on the identity of threadCallQueue (not that anyone should, but one never knows). If anyone thinks changing its identity should be avoided, I'd like this version:
def runUntilCurrent(self): """Run all pending timed calls. """ if self.threadCallQueue: calls = self.threadCallQueue[:] del self.threadCallQueue[:] for (f, a, kw) in calls: try: f(*a, **kw) except: log.err()
That's definitely racy! What if the call to callFromThread happens between the "calls = ..." and the del statement?
Shows how used to thinking about threaded code I am.
A version which does calls = self.threadCallQueue[:] del self.threadCallQueue[:len(calls)]
Should be ok though, for the same reason my patch is (or at least I think it is :)
The main difference between that version and my is this one involves a small list copy, but it might still be faster than executing "count += 1" on every loop iteration. Either way, it's at the point where I don't care :)
I think I like your version more now. Jp
participants (2)
-
Andrew Bennetts -
Jp Calderone