[jira] [Commented] (THRIFT-3932) C++ ThreadManager has a rare termination race

[ASF GitHub Bot (JIRA)]

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

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

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

    https://github.com/apache/thrift/pull/1103#discussion_r81467452
  
    --- Diff: lib/cpp/src/thrift/concurrency/ThreadManager.cpp ---
    @@ -339,103 +357,88 @@ void ThreadManager::Impl::addWorker(size_t value) {
         idMap_.insert(std::pair<const Thread::id_t, shared_ptr<Thread> 
>((*ix)->getId(), *ix));
       }
     
    -  {
    -    Synchronized s(workerMonitor_);
    -    while (workerCount_ != workerMaxCount_) {
    -      workerMonitor_.wait();
    -    }
    +  while (workerCount_ != workerMaxCount_) {
    +    workerMonitor_.wait();
       }
     }
     
     void ThreadManager::Impl::start() {
    -
    +  Guard g(mutex_);
       if (state_ == ThreadManager::STOPPED) {
         return;
       }
     
    -  {
    -    Synchronized s(monitor_);
    -    if (state_ == ThreadManager::UNINITIALIZED) {
    -      if (!threadFactory_) {
    -        throw InvalidArgumentException();
    -      }
    -      state_ = ThreadManager::STARTED;
    -      monitor_.notifyAll();
    +  if (state_ == ThreadManager::UNINITIALIZED) {
    +    if (!threadFactory_) {
    +      throw InvalidArgumentException();
         }
    +    state_ = ThreadManager::STARTED;
    +    monitor_.notifyAll();
    +  }
     
    -    while (state_ == STARTING) {
    -      monitor_.wait();
    -    }
    +  while (state_ == STARTING) {
    +    monitor_.wait();
       }
     }
     
    -void ThreadManager::Impl::stopImpl(bool join) {
    +void ThreadManager::Impl::stop() {
    +  Guard g(mutex_);
       bool doStop = false;
    -  if (state_ == ThreadManager::STOPPED) {
    -    return;
    -  }
     
    -  {
    -    Synchronized s(monitor_);
    -    if (state_ != ThreadManager::STOPPING && state_ != 
ThreadManager::JOINING
    -        && state_ != ThreadManager::STOPPED) {
    -      doStop = true;
    -      state_ = join ? ThreadManager::JOINING : ThreadManager::STOPPING;
    -    }
    +  if (state_ != ThreadManager::STOPPING && state_ != ThreadManager::JOINING
    +      && state_ != ThreadManager::STOPPED) {
    +    doStop = true;
    +    state_ = ThreadManager::JOINING;
       }
     
       if (doStop) {
    -    removeWorker(workerCount_);
    +    removeWorkersUnderLock(workerCount_);
       }
     
    -  // XXX
    -  // should be able to block here for transition to STOPPED since we're no
    -  // using shared_ptrs
    -
    -  {
    -    Synchronized s(monitor_);
    -    state_ = ThreadManager::STOPPED;
    -  }
    +  state_ = ThreadManager::STOPPED;
     }
     
     void ThreadManager::Impl::removeWorker(size_t value) {
    -  std::set<shared_ptr<Thread> > removedThreads;
    -  {
    -    Synchronized s(monitor_);
    -    if (value > workerMaxCount_) {
    -      throw InvalidArgumentException();
    -    }
    +  Guard g(mutex_);
    +  removeWorkersUnderLock(value);
    +}
     
    -    workerMaxCount_ -= value;
    +void ThreadManager::Impl::removeWorkersUnderLock(size_t value) {
    +  if (value > workerMaxCount_) {
    +    throw InvalidArgumentException();
    +  }
     
    -    if (idleCount_ < value) {
    -      for (size_t ix = 0; ix < idleCount_; ix++) {
    -        monitor_.notify();
    -      }
    -    } else {
    -      monitor_.notifyAll();
    +  workerMaxCount_ -= value;
    +
    +  if (idleCount_ < value) {
    +    for (size_t ix = 0; ix < idleCount_; ix++) {
    --- End diff --
    
    I know it's not your code but shouldn't it be
    
        if (idleCount_ > value) {
            for( ...; ix < value; ...)
    
    ?
    If insufficient or equal number of people are sleeping, let's wake them up 
all, otherwise do it one by one.


> C++ ThreadManager has a rare termination race
> ---------------------------------------------
>
>                 Key: THRIFT-3932
>                 URL: https://issues.apache.org/jira/browse/THRIFT-3932
>             Project: Thrift
>          Issue Type: Bug
>          Components: C++ - Library
>            Reporter: Buğra Gedik
>            Assignee: James E. King, III
>         Attachments: thrift-patch
>
>          Time Spent: 8h
>  Remaining Estimate: 0h
>
> {{ThreadManger::join}} calls {{stopImpl(true)}}, which in turn calls 
> {{removeWorker(workerCount_);}}. The latter waits until {{while (workerCount_ 
> != workerMaxCount_)}}. Within the {{run}} method of the workers, the last 
> thread that detects {{workerCount_ == workerMaxCount_}} notifies 
> {{removeWorker}}. The {{run}} method has the following additional code that 
> is executed at the very end:
> {code}
>     {
>       Synchronized s(manager_->workerMonitor_);
>       manager_->deadWorkers_.insert(this->thread());
>       if (notifyManager) {
>         manager_->workerMonitor_.notify();
>       }
>     }
> {code}
> This is an independent synchronized block. Now assume 2 threads. One of them 
> has {{notifyManager=true}} as it detected the {{workerCount_ == 
> workerMaxCount_}} condition earlier. It is possible that this thread gets to 
> execute  the above code block first, {{ThreadManager}}'s {{removeWorker}} 
> method unblocks, and eventually {{ThreadManager}}'s {{join}} returns and the 
> object is destructed. When the other thread reaches the synchronized block 
> above, it will crash, as the manager is not around anymore.
> Besides, {{ThreadManager}} never joins its threads.
> Attached is a patch.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)