ASF GitHub Bot commented on THRIFT-3932:

Github user ben-craig commented on the issue:

    PlatformThreadFactory f;
    Prior to this change, that was the only way to portably change the detached 
state.  The PosixThreadFactory didn't have a ctor that would permit that.  The 
tests were even using this pattern.
    I see that you have added a PosixThreadFactory ctor that lets you set the 
detached state in a portable way, and that's great.  It doesn't change that 
this pattern is already out there in a lot of production code.  It even broke 
tests which made no use of ThreadManager.
    If you need to fix the tests after making a change, chances are high that 
you have introduced a source breaking change that needs to be strongly 

> 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
>             Fix For: 0.11.0
>         Attachments: thrift-patch
>          Time Spent: 17h
>  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

Reply via email to