Github user tpcwang commented on a diff in the pull request:
https://github.com/apache/thrift/pull/980#discussion_r60612338
--- Diff: lib/cpp/src/thrift/concurrency/ThreadManager.cpp ---
@@ -421,7 +416,7 @@ void ThreadManager::Impl::removeWorker(size_t value) {
{
Synchronized s(workerMonitor_);
- while (workerCount_ != workerMaxCount_) {
+ while (workerCount_ > goalCount) {
--- End diff --
I'm not sure I understand what the problem is. Why would calling
removeWorker() at the same time clobber each other's desire goal? They have a
shared goal of reducing workerMaxCount_ and both will eventually get there,
right?
However, I think the new implementation does have problems with addWorker
and removeWorker. Say I start with 1 worker:
(1) Thread1 calls removeWorker(1) - goalCount is 0, workerCount_ is 1
(2) Thread2 calls addWorker(2) - goalCount is 0, workerCount_ is 3
(3) Thread3 worker is reaped - goalCount is 0, workerCount_ is 2 <- this
will stay at 2 forever
In any case, we may want to discuss this in a separate change if there is a
problem with the current implementation since this pull request no longer uses
the thread manager.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [email protected] or file a JIRA ticket
with INFRA.
---
