[ https://issues.apache.org/jira/browse/THRIFT-3932?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16153036#comment-16153036 ]
James E. King, III commented on THRIFT-3932: -------------------------------------------- This was fixed and released into thrift-0.10.0. > C++ ThreadManager has a rare termination race > --------------------------------------------- > > Key: THRIFT-3932 > URL: https://issues.apache.org/jira/browse/THRIFT-3932 > Project: Thrift > Issue Type: Bug > Components: C++ - Library > Reporter: Buğra Gedik > Assignee: James E. King, III > Fix For: 0.10.0 > > Attachments: thrift-patch > > Time Spent: 17h > Remaining Estimate: 0h > > {{ThreadManger::join}} calls {{stopImpl(true)}}, which in turn calls > {{removeWorker(workerCount_);}}. The latter waits until {{while (workerCount_ > != workerMaxCount_)}}. Within the {{run}} method of the workers, the last > thread that detects {{workerCount_ == workerMaxCount_}} notifies > {{removeWorker}}. The {{run}} method has the following additional code that > is executed at the very end: > {code} > { > Synchronized s(manager_->workerMonitor_); > manager_->deadWorkers_.insert(this->thread()); > if (notifyManager) { > manager_->workerMonitor_.notify(); > } > } > {code} > This is an independent synchronized block. Now assume 2 threads. One of them > has {{notifyManager=true}} as it detected the {{workerCount_ == > workerMaxCount_}} condition earlier. It is possible that this thread gets to > execute the above code block first, {{ThreadManager}}'s {{removeWorker}} > method unblocks, and eventually {{ThreadManager}}'s {{join}} returns and the > object is destructed. When the other thread reaches the synchronized block > above, it will crash, as the manager is not around anymore. > Besides, {{ThreadManager}} never joins its threads. > Attached is a patch. > {panel:title=Resolution > Details|borderStyle=dashed|borderColor=#ccc|titleBGColor=#F7D6C1|bgColor=#FFFFCE} > A fair number of things were improved with this pull request: > # There are three monitors used for signaling; two of them use the same mutex > and then the workerMonitor is using its own. I made all three use the same > mutex. > # I replaced all calls to Synchronize with Guard on the mutex_ so it is a bit > clearer that there is one lock protecting everything. > # The worker run loop expires tasks as it encounters them. Now we only call > removeExpiredTasks if there's no room during add. > # I found that when worker threads are removed they are not joined; if a > thread is not detached we now join those threads. > # I added better documentation and simplified the Worker::run() a little so > it is easier to understand, but the logic is the same. idle_ was not used and > removed. > # I found that remove(Task) simply wasn't implemented at all! so I added code > for that and tests. > # Release mode tests of thread manager would not fail on error because they > use assert() to verify results! If we only do release builds in CI, these > tests may have been silently failing for a long time. > # ThreadManager::join() is unnecessary, as stop() will join worker threads > (as will removeWorker) depending on the ThreadFactory's detached disposition. > # Cleaned up some technical debt from the TServerFramework refactoring: > # Cleaned up some things in the different platform thread factories. Some > abstractions were unnecessary. > # Reduced overall time on concurrency test to about 30 seconds on my dev > system while increasing the reliability and improving test coverage > significantly. I ran it 100 times in a loop. > # Added tests for most of the ThreadManager APIs. > # Tested with posix, std, and boost threads on linux. > # Tested on Windows. > # Fixed a math error in time calculation that was present on Windows (with > VS2010) which made expiring tasks impossible (unit test proved the issue and > the fix). > # Re-enable unit tests in the appveyor build that are no longer unstable. > {panel} -- This message was sent by Atlassian JIRA (v6.4.14#64029)