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

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

Github user ben-craig commented on the issue:

    https://github.com/apache/thrift/pull/1103
  
    Removing setDetached and the old thread factory ctors breaks client code 
loudly.  It doesn't fix the underlying problem, and it exchanges one problem 
(don't forget to set the detached mode!) for another (what does the 'false' in 
that parameter list mean?).
    
    I'm great with adding ctors that would make the code less buggy.  I'm also 
okay with the minor breakage of making isDetached and setDetached non-virtual.  
But removing setDetached just breaks too much code without a strong benefit.
    
    If your change ripples out to completely unrelated tests, you need a strong 
motivation for it.  Please separate out the refactors from the fixes.


> 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
(v6.3.4#6332)

Reply via email to