[GitHub] thrift pull request #1337: THRIFT-4292: Implement TimerManager::remove()

[jeking3 <gi...@git.apache.org>]

Github user jeking3 commented on a diff in the pull request:

    https://github.com/apache/thrift/pull/1337#discussion_r137163123
  
    --- 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;
    --- End diff --
    
    I'd recommend using the prefix/postfix method to walk and erase here in order to avoid iterator invalidation:
    
        bool found = false;
        for (task_iterator it = taskMap_.begin(); it != taskMap_.end(); ) {
          if (ix->second->runnable() == task) {
            found = true;
            taskMap_.erase(it++);
          } else {
            ++it;
          }
        }
    
        if (!found) {
          throw NoSuchTaskException();
        }



---