[Twisted-Python] INCOMPATIBLE CHANGE: twisted.python.threadpool
![](https://secure.gravatar.com/avatar/e1554622707bedd9202884900430b838.jpg?s=120&d=mm&r=g)
Hi all, (For those of you wondering about the subject, note that this message is sent in compliance with This email sent in compliance with <https://twistedmatrix.com/trac/wiki/CompatibilityPolicy#ProcedureforExceptio...>.) I've been trying to improve the reliability of the buildbots. (On that note, by the way, several builders are failing and more will be soon as security updates roll out, it would be great if someone could review <https://twistedmatrix.com/trac/ticket/7651> before the whole fleet turns red.) In so doing I (re-)discovered this bug: <https://twistedmatrix.com/trac/ticket/2673>. At first I just wanted to delete an intermittently failing test which appears to be nonsense. However, after some prodding from exarkun, I investigated and discovered that this very poorly-written test case does in fact illustrate a real problem with our current threadpool implementation which can result in deadlocks, hangs on exit, and other unpleasantness. In my use of Twisted I have encountered several "huh, this can't happen, maybe cosmic rays or something" bugs which might have been caused by this, so I would very much like to fix it. One reason that our threadpool implementation is problematic is that it has a somewhat complex internal design, lots of weird little features (context preservation, completion callbacks, workload estimation) all built at the same layer with no composition. I tried to figure out a way to salvage its current architecture and I could not reasonably do so. I have prototyped a completely new threadpool implementation ("twisted.threads") in a spike. It is definitely not ready for review, but I would definitely appreciate design-level feedback on the code right now. You can see the implementation here: <https://twistedmatrix.com/trac/browser/branches/desynchronize-2673/twisted/t...> (or here: <https://github.com/twisted/twisted/tree/desynchronize-2673/twisted/threads> if that's more your speed) It's less code than the existing implementation and provides more useful features at the same time. It might, in fact, provide some of the threading primitives that we would need for a reactor-per-thread implementation. However, there is a very ugly implementation detail that prevents us from marching onwards to a glorious multithreading future: twisted.internet.interfaces.IReactorThreads.getThreadPool. The nominally "public" interface provided by its documented return type, the concrete (old-style!) class twisted.python.threadpool.ThreadPool, is ridiculously over-broad. Here are the features (attributes and methods) in its "public" interface: callInThread callInThreadWithCallback adjustPoolsize start stop q min max joined started name waiters threads working workers start startAWorker stopAWorker dumpStats __getstate__ __setstate__ threadFactory currentThread Here's what I would like its public interface to be, more or less what it's intended to be, based on the ways it's documented and used in Twisted and in the various projects I can see using it: callInThread callInThreadWithCallback adjustPoolsize start stop It would not be too much effort to also preserve, for legacy purposes: min max joined started name dumpStats However, I would REALLY like to delete these implementation details: workers threads working startAWorker q __getstate__ __setstate__ threadFactory currentThread Deleting these implementation details straight up would also make it such that anyone who inherited from the public class ThreadPool would not be able to override any of these things (oh my goodness, why would you do that, I really hope nobody has done that). The three ways I could proceed, in order of my own preference, are: Put the new implementation with entirely new interfaces into twisted.threads, put a compatibility shim into twisted.python.threadpool.ThreadPool that wraps those objects, provides my more minimal interface proposed above, and deletes 90% of its tests. Change IReactorThreads.getThreadPool to return a documented interface considerably more restricted than Compatibility implications: twisted.internet.interfaces.IReactorThreads's contract would be relaxed. Implementors of IReactorThreads would not see an impact unless they were talking to deleted parts of ThreadPool internally. ThreadPool's clients would have several methods removed, and subclassing would effectively not be possible any more (a bunch of overridable hooks would have been removed, and no replacements would be provided). However, I prefer this option because there are an extremely restrictive set of use-cases (basically: only logging, any behavioral changes would have been broken) where clients could have made legitimate use of these attributes or overriding any of these functions. Delete all of twisted.python.threadpool's test coverage, deprecate the entire module, and delete it in the next release (14.1+1). This has the advantage of making the test suite reliable, and gets the benefits for any users of callInThread, but does not relay any of the benefits to users of ThreadPool. The only ones doing this directly in Twisted are deferToThreadPool (and by extension, deferToThread) and WSGIResource. I could update these in tandem, however; the code changes would be very small. I don't think it would be incompatible to make WSGIResource itself accept a different type of constructor argument (although if we don't, I wonder if subclassing anything with a constructor can be considered applicable to the compatibility policy in Twisted; hmm). Actually go through the trouble of emulating all the attributes on ThreadPool and emulating them as best I can. Delete the direct tests for ThreadPool itself and write a private subclass that will be returned from getThreadPool that returns a compatibility shim which isinstance(ThreadPool) for compatibility and still has all the gross attributes. This change would be technically compatible, but would be a huge amount of additional work and I am doubtful that it would provide any value. So, does anyone out there have any code which makes use of the aforementioned bad attributes of ThreadPool, whose applications would break if I removed them? If so, how can I make you feel as bad about yourselves for using it as I feel bad about myself for writing it? ;-) -glyph
![](https://secure.gravatar.com/avatar/e6ab4a797e4c2dfeae6210862c3c4543.jpg?s=120&d=mm&r=g)
On Sep 25, 2014, at 3:31 PM, Glyph Lefkowitz <glyph@twistedmatrix.com> wrote:
So, does anyone out there have any code which makes use of the aforementioned bad attributes of ThreadPool, whose applications would break if I removed them?
Yes. Specifically, I am maintaining this AMP responder method: @FetchThreadPoolStats.responder def fetchThreadPoolStats(self): pool = self.factory.threadPool return dict( threadsWaiting=len(pool.waiters), threadsWorking=len(pool.working), workerQueueSize=pool.q.qsize(), ) These statistics are chunked into graphite and displayed as a nice little graph. They’ve been quite useful on some occasions: sometimes all of the threads in a thread pool for a WSGI application got blocked, and the symptoms were that all of the requests coming in were timing out at the proxy in front of twisted. We were able to quickly tell that the problem was the WSGI thread pool because the threadsWorking count was at its limit and the workerQueueSize was skyrocketing.
If so, how can I make you feel as bad about yourselves for using it as I feel bad about myself for writing it? ;-)
I don’t really see this as an abuse of the public interface. If possible, I’d like to see this diagnostic information preserved in the new interface, as I consider it invaluable information as long as we’re using twisted as a WSGI runner. I will say that it is certainly possible for me to emulate these attributes, but that would require touching implementation details of WSGIResource. I’m not sure which is worse. ~weykent
![](https://secure.gravatar.com/avatar/e1554622707bedd9202884900430b838.jpg?s=120&d=mm&r=g)
On Sep 25, 2014, at 6:36 PM, weykent <weykent@weasyl.com> wrote:
Wow, cool. So, okay, while I am a little unhappy that you're using this API in a slightly unfortunate way, that feeling is eclipsed by the joy that The Process Works :-). Thank you very much for responding so promptly, so specifically, and including the exact code that we need to discuss.
Analytics, monitoring, logging, and statistics have historically been a blind spot for Twisted and I am really glad you're bringing this up. I was thinking about bringing it up in my original message, but as my colleagues and friends will tell you, I tend to put too many words into emails, so unless I'm sure I need those words, I will delete them. The new interface should very definitely have metrics-gathering functionality, and possibly even a logging component.
If so, how can I make you feel as bad about yourselves for using it as I feel bad about myself for writing it? ;-)
I don’t really see this as an abuse of the public interface. If possible, I’d like to see this diagnostic information preserved in the new interface, as I consider it invaluable information as long as we’re using twisted as a WSGI runner.
It's not exactly an "abuse", because the public interface is there, and you have a use-case, and you're satisfying it with what's available. For the purposes that you are using these attributes, I actually don't have a problem reproducing them at all. It should be pretty straightforward. My concern with exposing them overall is that what you've got with "ThreadPool.q" is not a queue that has a qsize() method that you can inspect, it's that it's a Queue that you can put things into, get things out of, and generally muck about with. Similarly pool.waiters is not simply an object with a __len__ that tells you how active the threads are, it's a list of Thread objects which you could potentially change the names of, turn into daemon threads, join(), and so on. More importantly it's a list that you could remove Thread objects from and that would affect their interactions with the pool. How about this: would you mind if ThreadPool were modified to still populate these attributes for monitoring and debugging purposes, but completely stopped honoring any destructive operations on them? In the case of "pool.q", there is no single Queue responsible for the whole pool, so I would like to make "put", "get", and "join" actually raise exceptions; in the case of "waiters" and "working" I guess I could fish out some actual Thread objects, that just involves a little bit of sad abstraction violation internal to the twisted.threads.Team implementation. My inclination would be to expand the thread pool to accommodate these usages, but still deprecate these attributes in the next version; but add a better "statistics" method that returned an object with "active", "pending", and "idle" attributes (all integers). The most important thing that I want to make sure nobody wants is to keep overriding (or calling) startAWorker, stopAWorker, __getstate__, __setstate__. Supporting callers of threadFactory and currentThread would be easy, but the attributes aren't really there for calling, they're there to be stubbed, and stubbing them won't keep working without tons of extra work. So would that work for you?
I will say that it is certainly possible for me to emulate these attributes, but that would require touching implementation details of WSGIResource. I’m not sure which is worse.
Touching private (underscore-prefixed) implementation details is explicitly not supported - if you did it that way, you're on your own :-). And, as the end-user in this discussion, you win the argument by default. (You can see how I feel about supporting users here: <https://twitter.com/mjg59/status/514670470260465664>) Plus, if we're going to work on the thread-pool and have newer, better interfaces, WSGIResource's implementation is likely to change. Thanks a bunch for helping us improve Twisted, -glyph
![](https://secure.gravatar.com/avatar/e6ab4a797e4c2dfeae6210862c3c4543.jpg?s=120&d=mm&r=g)
On Sep 26, 2014, at 12:43 AM, Glyph <glyph@twistedmatrix.com> wrote:
How about this: would you mind if ThreadPool were modified to still populate these attributes for monitoring and debugging purposes, but completely stopped honoring any destructive operations on them? In the case of "pool.q", there is no single Queue responsible for the whole pool, so I would like to make "put", "get", and "join" actually raise exceptions; in the case of "waiters" and "working" I guess I could fish out some actual Thread objects, that just involves a little bit of sad abstraction violation internal to the twisted.threads.Team implementation.
Having objects with only nondestructive methods would be fine for me. I don’t even care about getting Thread objects out, and I certainly hope nobody else cares about getting Thread objects out either.
My inclination would be to expand the thread pool to accommodate these usages, but still deprecate these attributes in the next version; but add a better "statistics" method that returned an object with "active", "pending", and "idle" attributes (all integers).
This sounds fantastic. I’ll graph whatever the statistics method emits, so feel free to include other useful or interesting metrics as well. (Though, I’m not sure what else there is that’s easy to keep track of and useful.)
So would that work for you?
Yes, absolutely.
Touching private (underscore-prefixed) implementation details is explicitly not supported - if you did it that way, you're on your own :-). And, as the end-user in this discussion, you win the argument by default. (You can see how I feel about supporting users here: <https://twitter.com/mjg59/status/514670470260465664>) Plus, if we're going to work on the thread-pool and have newer, better interfaces, WSGIResource's implementation is likely to change.
I haven’t touched private implementation details of WSGIResource, nor was I planning to. I’ve already had some code (in another project) break across versions of twisted because of that, and I don’t want it to happen again.
Thanks a bunch for helping us improve Twisted,
Thank you for taking the time to write up your original post and this reply! ~weykent
![](https://secure.gravatar.com/avatar/c194a4d2f2f8269aa052942e87985198.jpg?s=120&d=mm&r=g)
On 26 September 2014 08:43, Glyph <glyph@twistedmatrix.com> wrote: [snip]
+1 for statistics method I am using some of those member for my custom test case https://github.com/chevah/empirical/blob/master/chevah/empirical/testcase.py... A method which return those integers would be enough for my use case. ---- +1 for reducing the public interface. With a public adjustPoolsize I prefer to also have read-only attributes for min / max pool size. With a public start/stop a public read-only started attribute might be useful.... for the case when you don't want to do a try/catch or just want to see the state of the pool. dumpStats could be replaced by a better method dedicated to statistics, which will return integer instead of using the logger. `joined` looks like the opposite of started/running... ie, it is redundant. Regarding `name` attribute, I guess that it might make sense in the case an application uses multiple thread pools... I have never used multiple pools so I can not comment on that. Thanks! -- Adi Roiban
![](https://secure.gravatar.com/avatar/3a7e70f3ef2ad1539da42afc85c8d09d.jpg?s=120&d=mm&r=g)
On September 25, 2014 at 8:39:54 PM, weykent (weykent@weasyl.com) wrote: On Sep 25, 2014, at 3:31 PM, Glyph Lefkowitz <glyph@twistedmatrix.com> wrote:
So, does anyone out there have any code which makes use of the aforementioned bad attributes of ThreadPool, whose applications would break if I removed them?
Yes. Specifically, I am maintaining this AMP responder method: @FetchThreadPoolStats.responder def fetchThreadPoolStats(self): pool = self.factory.threadPool return dict( threadsWaiting=len(pool.waiters), threadsWorking=len(pool.working), workerQueueSize=pool.q.qsize(), ) These statistics are chunked into graphite and displayed as a nice little graph. They’ve been quite useful on some occasions: sometimes all of the threads in a thread pool for a WSGI application got blocked, and the symptoms were that all of the requests coming in were timing out at the proxy in front of twisted. We were able to quickly tell that the problem was the WSGI thread pool because the threadsWorking count was at its limit and the workerQueueSize was skyrocketing. We did very similar stuff (logging thread pool size to graphite) at id, so they're probably still using those bits too.
If so, how can I make you feel as bad about yourselves for using it as I feel bad about myself for writing it? ;-)
I don’t really see this as an abuse of the public interface. If possible, I’d like to see this diagnostic information preserved in the new interface, as I consider it invaluable information as long as we’re using twisted as a WSGI runner. I will say that it is certainly possible for me to emulate these attributes, but that would require touching implementation details of WSGIResource. I’m not sure which is worse. ~weykent -- Christopher Armstrong http://twitter.com/radix http://wordeology.com/
![](https://secure.gravatar.com/avatar/0bc9e314eaf82ddbbcf40fd8f8b30b11.jpg?s=120&d=mm&r=g)
On 26 September 2014 08:31, Glyph Lefkowitz <glyph@twistedmatrix.com> wrote:
There are a few times in a codebase I help maintain where we want a thread pool of all daemon threads. The code we have for this overrides threadFactory() like so: class DaemonThreadPool(ThreadPool): def threadFactory(self, *args, **kwargs): t = threading.Thread(*args, **kwargs) t.setDaemon(True) return t How would I do this with your proposed new API? Josh.
![](https://secure.gravatar.com/avatar/e1554622707bedd9202884900430b838.jpg?s=120&d=mm&r=g)
On Sep 28, 2014, at 6:56 PM, Joshua Bartlett <josh@bartletts.id.au> wrote:
I suppose honoring the threadFactory attribute is possible. In the new thread pool prototype I've created so far, "Team", provides pluggability for creating different kinds of "Worker": <https://github.com/twisted/twisted/blob/desynchronize-2673/twisted/threads/_...> and the thread "Worker" provides pluggability for creating different kinds of thread: <https://github.com/twisted/twisted/blob/desynchronize-2673/twisted/threads/_...> so I'll make sure that this keeps working. That said: why did you need a threadpool of daemon threads? -glyph
![](https://secure.gravatar.com/avatar/0bc9e314eaf82ddbbcf40fd8f8b30b11.jpg?s=120&d=mm&r=g)
On 1 October 2014 14:09, Glyph <glyph@twistedmatrix.com> wrote:
That said: why did you need a threadpool of daemon threads?
That's a very good question. One of my comaintainers thinks the answer is, "My codebase is old enough to vote and to answer your question I'd probably have to turn our SVN server back on. Please don't make me." :-) I think the real reason is that the third-party library that we're calling from the threads sometimes takes a really long time to return, which means that unless we use daemon threads the only timely way to shut down / restart the service is kill -9, which is not ideal. Josh.
![](https://secure.gravatar.com/avatar/e1554622707bedd9202884900430b838.jpg?s=120&d=mm&r=g)
On Sep 30, 2014, at 10:12 PM, Joshua Bartlett <josh@bartletts.id.au> wrote:
Aah. That's a very good reason, come to think of it. Maybe we should default to daemon threads at some point in the future, for that matter; it would make exiting easier, by allowing us to still join() all the threads, but still exit if attempting to do so times out. -glyph
![](https://secure.gravatar.com/avatar/e6ab4a797e4c2dfeae6210862c3c4543.jpg?s=120&d=mm&r=g)
On Sep 25, 2014, at 3:31 PM, Glyph Lefkowitz <glyph@twistedmatrix.com> wrote:
So, does anyone out there have any code which makes use of the aforementioned bad attributes of ThreadPool, whose applications would break if I removed them?
Yes. Specifically, I am maintaining this AMP responder method: @FetchThreadPoolStats.responder def fetchThreadPoolStats(self): pool = self.factory.threadPool return dict( threadsWaiting=len(pool.waiters), threadsWorking=len(pool.working), workerQueueSize=pool.q.qsize(), ) These statistics are chunked into graphite and displayed as a nice little graph. They’ve been quite useful on some occasions: sometimes all of the threads in a thread pool for a WSGI application got blocked, and the symptoms were that all of the requests coming in were timing out at the proxy in front of twisted. We were able to quickly tell that the problem was the WSGI thread pool because the threadsWorking count was at its limit and the workerQueueSize was skyrocketing.
If so, how can I make you feel as bad about yourselves for using it as I feel bad about myself for writing it? ;-)
I don’t really see this as an abuse of the public interface. If possible, I’d like to see this diagnostic information preserved in the new interface, as I consider it invaluable information as long as we’re using twisted as a WSGI runner. I will say that it is certainly possible for me to emulate these attributes, but that would require touching implementation details of WSGIResource. I’m not sure which is worse. ~weykent
![](https://secure.gravatar.com/avatar/e1554622707bedd9202884900430b838.jpg?s=120&d=mm&r=g)
On Sep 25, 2014, at 6:36 PM, weykent <weykent@weasyl.com> wrote:
Wow, cool. So, okay, while I am a little unhappy that you're using this API in a slightly unfortunate way, that feeling is eclipsed by the joy that The Process Works :-). Thank you very much for responding so promptly, so specifically, and including the exact code that we need to discuss.
Analytics, monitoring, logging, and statistics have historically been a blind spot for Twisted and I am really glad you're bringing this up. I was thinking about bringing it up in my original message, but as my colleagues and friends will tell you, I tend to put too many words into emails, so unless I'm sure I need those words, I will delete them. The new interface should very definitely have metrics-gathering functionality, and possibly even a logging component.
If so, how can I make you feel as bad about yourselves for using it as I feel bad about myself for writing it? ;-)
I don’t really see this as an abuse of the public interface. If possible, I’d like to see this diagnostic information preserved in the new interface, as I consider it invaluable information as long as we’re using twisted as a WSGI runner.
It's not exactly an "abuse", because the public interface is there, and you have a use-case, and you're satisfying it with what's available. For the purposes that you are using these attributes, I actually don't have a problem reproducing them at all. It should be pretty straightforward. My concern with exposing them overall is that what you've got with "ThreadPool.q" is not a queue that has a qsize() method that you can inspect, it's that it's a Queue that you can put things into, get things out of, and generally muck about with. Similarly pool.waiters is not simply an object with a __len__ that tells you how active the threads are, it's a list of Thread objects which you could potentially change the names of, turn into daemon threads, join(), and so on. More importantly it's a list that you could remove Thread objects from and that would affect their interactions with the pool. How about this: would you mind if ThreadPool were modified to still populate these attributes for monitoring and debugging purposes, but completely stopped honoring any destructive operations on them? In the case of "pool.q", there is no single Queue responsible for the whole pool, so I would like to make "put", "get", and "join" actually raise exceptions; in the case of "waiters" and "working" I guess I could fish out some actual Thread objects, that just involves a little bit of sad abstraction violation internal to the twisted.threads.Team implementation. My inclination would be to expand the thread pool to accommodate these usages, but still deprecate these attributes in the next version; but add a better "statistics" method that returned an object with "active", "pending", and "idle" attributes (all integers). The most important thing that I want to make sure nobody wants is to keep overriding (or calling) startAWorker, stopAWorker, __getstate__, __setstate__. Supporting callers of threadFactory and currentThread would be easy, but the attributes aren't really there for calling, they're there to be stubbed, and stubbing them won't keep working without tons of extra work. So would that work for you?
I will say that it is certainly possible for me to emulate these attributes, but that would require touching implementation details of WSGIResource. I’m not sure which is worse.
Touching private (underscore-prefixed) implementation details is explicitly not supported - if you did it that way, you're on your own :-). And, as the end-user in this discussion, you win the argument by default. (You can see how I feel about supporting users here: <https://twitter.com/mjg59/status/514670470260465664>) Plus, if we're going to work on the thread-pool and have newer, better interfaces, WSGIResource's implementation is likely to change. Thanks a bunch for helping us improve Twisted, -glyph
![](https://secure.gravatar.com/avatar/e6ab4a797e4c2dfeae6210862c3c4543.jpg?s=120&d=mm&r=g)
On Sep 26, 2014, at 12:43 AM, Glyph <glyph@twistedmatrix.com> wrote:
How about this: would you mind if ThreadPool were modified to still populate these attributes for monitoring and debugging purposes, but completely stopped honoring any destructive operations on them? In the case of "pool.q", there is no single Queue responsible for the whole pool, so I would like to make "put", "get", and "join" actually raise exceptions; in the case of "waiters" and "working" I guess I could fish out some actual Thread objects, that just involves a little bit of sad abstraction violation internal to the twisted.threads.Team implementation.
Having objects with only nondestructive methods would be fine for me. I don’t even care about getting Thread objects out, and I certainly hope nobody else cares about getting Thread objects out either.
My inclination would be to expand the thread pool to accommodate these usages, but still deprecate these attributes in the next version; but add a better "statistics" method that returned an object with "active", "pending", and "idle" attributes (all integers).
This sounds fantastic. I’ll graph whatever the statistics method emits, so feel free to include other useful or interesting metrics as well. (Though, I’m not sure what else there is that’s easy to keep track of and useful.)
So would that work for you?
Yes, absolutely.
Touching private (underscore-prefixed) implementation details is explicitly not supported - if you did it that way, you're on your own :-). And, as the end-user in this discussion, you win the argument by default. (You can see how I feel about supporting users here: <https://twitter.com/mjg59/status/514670470260465664>) Plus, if we're going to work on the thread-pool and have newer, better interfaces, WSGIResource's implementation is likely to change.
I haven’t touched private implementation details of WSGIResource, nor was I planning to. I’ve already had some code (in another project) break across versions of twisted because of that, and I don’t want it to happen again.
Thanks a bunch for helping us improve Twisted,
Thank you for taking the time to write up your original post and this reply! ~weykent
![](https://secure.gravatar.com/avatar/c194a4d2f2f8269aa052942e87985198.jpg?s=120&d=mm&r=g)
On 26 September 2014 08:43, Glyph <glyph@twistedmatrix.com> wrote: [snip]
+1 for statistics method I am using some of those member for my custom test case https://github.com/chevah/empirical/blob/master/chevah/empirical/testcase.py... A method which return those integers would be enough for my use case. ---- +1 for reducing the public interface. With a public adjustPoolsize I prefer to also have read-only attributes for min / max pool size. With a public start/stop a public read-only started attribute might be useful.... for the case when you don't want to do a try/catch or just want to see the state of the pool. dumpStats could be replaced by a better method dedicated to statistics, which will return integer instead of using the logger. `joined` looks like the opposite of started/running... ie, it is redundant. Regarding `name` attribute, I guess that it might make sense in the case an application uses multiple thread pools... I have never used multiple pools so I can not comment on that. Thanks! -- Adi Roiban
![](https://secure.gravatar.com/avatar/3a7e70f3ef2ad1539da42afc85c8d09d.jpg?s=120&d=mm&r=g)
On September 25, 2014 at 8:39:54 PM, weykent (weykent@weasyl.com) wrote: On Sep 25, 2014, at 3:31 PM, Glyph Lefkowitz <glyph@twistedmatrix.com> wrote:
So, does anyone out there have any code which makes use of the aforementioned bad attributes of ThreadPool, whose applications would break if I removed them?
Yes. Specifically, I am maintaining this AMP responder method: @FetchThreadPoolStats.responder def fetchThreadPoolStats(self): pool = self.factory.threadPool return dict( threadsWaiting=len(pool.waiters), threadsWorking=len(pool.working), workerQueueSize=pool.q.qsize(), ) These statistics are chunked into graphite and displayed as a nice little graph. They’ve been quite useful on some occasions: sometimes all of the threads in a thread pool for a WSGI application got blocked, and the symptoms were that all of the requests coming in were timing out at the proxy in front of twisted. We were able to quickly tell that the problem was the WSGI thread pool because the threadsWorking count was at its limit and the workerQueueSize was skyrocketing. We did very similar stuff (logging thread pool size to graphite) at id, so they're probably still using those bits too.
If so, how can I make you feel as bad about yourselves for using it as I feel bad about myself for writing it? ;-)
I don’t really see this as an abuse of the public interface. If possible, I’d like to see this diagnostic information preserved in the new interface, as I consider it invaluable information as long as we’re using twisted as a WSGI runner. I will say that it is certainly possible for me to emulate these attributes, but that would require touching implementation details of WSGIResource. I’m not sure which is worse. ~weykent -- Christopher Armstrong http://twitter.com/radix http://wordeology.com/
![](https://secure.gravatar.com/avatar/0bc9e314eaf82ddbbcf40fd8f8b30b11.jpg?s=120&d=mm&r=g)
On 26 September 2014 08:31, Glyph Lefkowitz <glyph@twistedmatrix.com> wrote:
There are a few times in a codebase I help maintain where we want a thread pool of all daemon threads. The code we have for this overrides threadFactory() like so: class DaemonThreadPool(ThreadPool): def threadFactory(self, *args, **kwargs): t = threading.Thread(*args, **kwargs) t.setDaemon(True) return t How would I do this with your proposed new API? Josh.
![](https://secure.gravatar.com/avatar/e1554622707bedd9202884900430b838.jpg?s=120&d=mm&r=g)
On Sep 28, 2014, at 6:56 PM, Joshua Bartlett <josh@bartletts.id.au> wrote:
I suppose honoring the threadFactory attribute is possible. In the new thread pool prototype I've created so far, "Team", provides pluggability for creating different kinds of "Worker": <https://github.com/twisted/twisted/blob/desynchronize-2673/twisted/threads/_...> and the thread "Worker" provides pluggability for creating different kinds of thread: <https://github.com/twisted/twisted/blob/desynchronize-2673/twisted/threads/_...> so I'll make sure that this keeps working. That said: why did you need a threadpool of daemon threads? -glyph
![](https://secure.gravatar.com/avatar/0bc9e314eaf82ddbbcf40fd8f8b30b11.jpg?s=120&d=mm&r=g)
On 1 October 2014 14:09, Glyph <glyph@twistedmatrix.com> wrote:
That said: why did you need a threadpool of daemon threads?
That's a very good question. One of my comaintainers thinks the answer is, "My codebase is old enough to vote and to answer your question I'd probably have to turn our SVN server back on. Please don't make me." :-) I think the real reason is that the third-party library that we're calling from the threads sometimes takes a really long time to return, which means that unless we use daemon threads the only timely way to shut down / restart the service is kill -9, which is not ideal. Josh.
![](https://secure.gravatar.com/avatar/e1554622707bedd9202884900430b838.jpg?s=120&d=mm&r=g)
On Sep 30, 2014, at 10:12 PM, Joshua Bartlett <josh@bartletts.id.au> wrote:
Aah. That's a very good reason, come to think of it. Maybe we should default to daemon threads at some point in the future, for that matter; it would make exiting easier, by allowing us to still join() all the threads, but still exit if attempting to do so times out. -glyph
participants (7)
-
Adi Roiban
-
Christopher Armstrong
-
Glyph
-
Glyph Lefkowitz
-
Jonathan Lange
-
Joshua Bartlett
-
weykent