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

[Typz]

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

    https://github.com/apache/thrift/pull/1337#discussion_r137808236
  
    --- 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 --
    
    Tasks cannot exist multiple times, only newly allocated tasks are inserted:
    ```cpp
      taskMap_.insert(
            std::pair<int64_t, shared_ptr<Task> >(timeout, shared_ptr<Task>(new 
Task(task))));
    ```
    
    Which is why I implemented it in the same fashion as the run() method. But 
i can change it.


---