[
https://issues.apache.org/jira/browse/THRIFT-3932?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15540326#comment-15540326
]
ASF GitHub Bot commented on THRIFT-3932:
----------------------------------------
Github user jeking3 commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1103#discussion_r81469729
--- Diff: lib/cpp/src/thrift/concurrency/ThreadManager.cpp ---
@@ -339,103 +357,88 @@ void ThreadManager::Impl::addWorker(size_t value) {
idMap_.insert(std::pair<const Thread::id_t, shared_ptr<Thread>
>((*ix)->getId(), *ix));
}
- {
- Synchronized s(workerMonitor_);
- while (workerCount_ != workerMaxCount_) {
- workerMonitor_.wait();
- }
+ while (workerCount_ != workerMaxCount_) {
+ workerMonitor_.wait();
}
}
void ThreadManager::Impl::start() {
-
+ Guard g(mutex_);
if (state_ == ThreadManager::STOPPED) {
return;
}
- {
- Synchronized s(monitor_);
- if (state_ == ThreadManager::UNINITIALIZED) {
- if (!threadFactory_) {
- throw InvalidArgumentException();
- }
- state_ = ThreadManager::STARTED;
- monitor_.notifyAll();
+ if (state_ == ThreadManager::UNINITIALIZED) {
+ if (!threadFactory_) {
+ throw InvalidArgumentException();
}
+ state_ = ThreadManager::STARTED;
+ monitor_.notifyAll();
+ }
- while (state_ == STARTING) {
- monitor_.wait();
- }
+ while (state_ == STARTING) {
+ monitor_.wait();
}
}
-void ThreadManager::Impl::stopImpl(bool join) {
+void ThreadManager::Impl::stop() {
+ Guard g(mutex_);
bool doStop = false;
- if (state_ == ThreadManager::STOPPED) {
- return;
- }
- {
- Synchronized s(monitor_);
- if (state_ != ThreadManager::STOPPING && state_ !=
ThreadManager::JOINING
- && state_ != ThreadManager::STOPPED) {
- doStop = true;
- state_ = join ? ThreadManager::JOINING : ThreadManager::STOPPING;
- }
+ if (state_ != ThreadManager::STOPPING && state_ != ThreadManager::JOINING
+ && state_ != ThreadManager::STOPPED) {
+ doStop = true;
+ state_ = ThreadManager::JOINING;
}
if (doStop) {
- removeWorker(workerCount_);
+ removeWorkersUnderLock(workerCount_);
}
- // XXX
- // should be able to block here for transition to STOPPED since we're no
- // using shared_ptrs
-
- {
- Synchronized s(monitor_);
- state_ = ThreadManager::STOPPED;
- }
+ state_ = ThreadManager::STOPPED;
}
void ThreadManager::Impl::removeWorker(size_t value) {
- std::set<shared_ptr<Thread> > removedThreads;
- {
- Synchronized s(monitor_);
- if (value > workerMaxCount_) {
- throw InvalidArgumentException();
- }
+ Guard g(mutex_);
+ removeWorkersUnderLock(value);
+}
- workerMaxCount_ -= value;
+void ThreadManager::Impl::removeWorkersUnderLock(size_t value) {
+ if (value > workerMaxCount_) {
+ throw InvalidArgumentException();
+ }
- if (idleCount_ < value) {
- for (size_t ix = 0; ix < idleCount_; ix++) {
- monitor_.notify();
- }
- } else {
- monitor_.notifyAll();
+ workerMaxCount_ -= value;
+
+ if (idleCount_ < value) {
+ for (size_t ix = 0; ix < idleCount_; ix++) {
--- End diff --
I will look at this more closely.
> 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
> Attachments: thrift-patch
>
> Time Spent: 8h
> 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.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
