On May 10, 2006 12:14:24 AM -0700 Frank Cusack <fcusack@fcusack.com> wrote:
I was wondering why threads.c:thread_pool_clean() doesn't need a mutex when calling delete_thread().
There is a comment just below that, that a mutex isn't needed to read "the location" -- which I take to mean thread_pool.active_threads -- as it's only needed to modify the location.
However, just below that, spawn_thread() is called, which does modify thread_pool.active_threads and a mutex isn't held to do that.
From a quick read (and late at night), ISTM that this is just an error in documentation.
a) Only the main thread deals with active_threads and total_threads (this is a rough guess, not thoroughly explored) b) There isn't even a mutex to be locked! thread_pool has 2 mutexes, they are used for different things.
Sound about right? Or is there indeed some missing locking.
After some more study, I'm a little bit off in my explanation. This is the kind of thing that's way easier on a whiteboard, but anyway ... spawn_thread() modifies total_threads, not active_threads, so this is ok. (AFAICT total_threads is only used by the main thread.) active_threads is protected by the queue mutex (should probably be better documented), and in that case I agree with the comment in t_p_clean(). Alan or someone please confirm or argue. But I have a new question. thread_pool_addrequest() uses active_threads to determine if it should spawn, but it doesn't hold the queue mutex. So it could err either way. I'd argue that the log message can be bogus but if you're that much against the limit it doesn't matter. There should just be a comment there about possible stale value. And one last question. Why does total_active_threads walk the entire thread pool? It should just return active_threads. Either way it can return a stale value. I'd go ahead and make those changes but that's hairy part of the server so my confidence about being correct isn't super high. -frank