thrift-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "ASF GitHub Bot (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (THRIFT-4292) TimerManager::remove() is not implemented
Date Wed, 06 Sep 2017 03:20:01 GMT

    [ https://issues.apache.org/jira/browse/THRIFT-4292?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16154751#comment-16154751
] 

ASF GitHub Bot commented on THRIFT-4292:
----------------------------------------

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.


> TimerManager::remove() is not implemented
> -----------------------------------------
>
>                 Key: THRIFT-4292
>                 URL: https://issues.apache.org/jira/browse/THRIFT-4292
>             Project: Thrift
>          Issue Type: Bug
>          Components: C++ - Library
>            Reporter: Francois Ferrand
>
> The function is currently not implemented.
> This is not critical for Thrift, since it is not used there, but prevents using it in
thrift-based code.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

Mime
View raw message