Github user jeking3 commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1337#discussion_r137162542
--- Diff: lib/cpp/src/thrift/concurrency/TimerManager.cpp ---
@@ -290,11 +292,23 @@ void TimerManager::add(shared_ptr<Runnable> task,
const struct timeval& value) {
}
void TimerManager::remove(shared_ptr<Runnable> task) {
- (void)task;
Synchronized s(monitor_);
if (state_ != TimerManager::STARTED) {
throw IllegalStateException();
}
+ std::vector<task_iterator> toRemove;
+ for (task_iterator ix = taskMap_.begin(); ix != taskMap_.end(); ++ix) {
+ if (ix->second->runnable() == task) {
+ toRemove.push_back(ix);
+ }
+ }
+ if (toRemove.empty()) {
+ throw NoSuchTaskException();
+ }
+ for (std::vector<task_iterator>::iterator ix = toRemove.begin(); ix !=
toRemove.end(); ++ix) {
+ taskMap_.erase(*ix);
--- End diff --
As written, if a single task exists multiple times in the taskMap then this
first erase call invalidates any other iterators in toRemove, and it will lead
to undefined behavior. This must be fixed to merge; see my previous comment for
a safer way to achieve this.
---
