thrift-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From jeking3 <...@git.apache.org>
Subject [GitHub] thrift pull request #1337: THRIFT-4292: Implement TimerManager::remove()
Date Wed, 06 Sep 2017 03:19:25 GMT
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.


---

Mime
View raw message