Bug #12444
closedWorkQueue.cc: unintended behavior for ThreadPool::drain()?
0%
Description
ThreadPool::drain() contains this condition:
while (processing || (wq != NULL && !wq->_empty())) _wait_cond.Wait(_lock);
This will wait until no threads are processing (which only happens when all queues are empty), AND the queue given by wq is empty (if wq is not NULL). However, as far as I can tell, the intent of the API seems to be that drain(wq) should only wait until the queue given in the parameter is empty, since otherwise passing a work queue which is already added to the thread pool (as seen in e.g. WorkQueue::drain()) is equivalent to passing NULL. Therefore the correct condition seems to be:
while (processing && (wq == NULL || wq->_empty())) _wait_cond.Wait(_lock);
Am I missing something here?
Updated by Kefu Chai over 8 years ago
- Status changed from New to 12
Krzysztof,
thanks for pointing this out. I think you are right. the existing ThreadPool::drain(wq) will wait until all the work queues added to this pool are empty if the thread pool, which renders the wq parameter useless.
i will run some qa suite against the patch proposed by you. and see how it works.
Updated by Krzysztof Kosinski over 8 years ago
I looked at the condition I wrote again and it seems like it's wrong, should actually be:
while (processing && (wq == NULL || !wq->_empty()))
_wait_cond.Wait(_lock);(Missing `!` before `wq->_empty()`)
Better yet, use an explicit `if`, because we end up with double negatives and the condition becomes very confusing:
if (wq != NULL) {
while(!wq->_empty())
_wait_cond.Wait(_lock);
} else {
while(processing)
_wait_cond.Wait(_lock);
}Updated by Greg Farnum almost 7 years ago
Kefu, ever look at this?
Without much context I think the condition is correct as originally written: we keep waiting while there is an in-flight op, and if we got a specific WorkQueue, we wait until that workqueue is drained.
Updated by Kefu Chai almost 7 years ago
no, once the specific wq is drained. ThreadPool::drain() should return, but in current implementation, the loop won't stop until processing is false, which means it will stop only if none job is being processed and wq is drained.
Updated by Greg Farnum almost 7 years ago
Yeah, but I don't think we can avoid that — processing is a count of how many threads are currently doing one of the THreadPool's ops. We don't have a per-WorkQueue equivalent, so any in-flight op might belong to the WorkQueue we're waiting on, which means we need to wait for it.
It does seem like we might need to draw a bit more of a distinction here though, since _draining won't stop a thread from doing other work. So there's an issue, but the proposed code definitely isn't the solution — it's not waiting when there are items in the WorkQueue. ;)
Updated by Kefu Chai almost 7 years ago
- Status changed from 12 to Rejected
Greg, i concur with you aside from following statement:
it's not waiting when there are items in the WorkQueue. ;)
IMHO, it should be
it's not waiting when there are in-flight ops which might belong to the WorkQueue.
anyway, i am closing this ticket. as
We don't have a per-WorkQueue equivalent