[
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)
