[ https://issues.apache.org/jira/browse/THRIFT-3038?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15660913#comment-15660913 ]
ASF GitHub Bot commented on THRIFT-3038: ---------------------------------------- GitHub user jeking3 opened a pull request: https://github.com/apache/thrift/pull/1129 THRIFT-3038 clean up remaining volatile issues; remove unused NoStarveReadWriteMutex class This takes care of items 1, 3, and 6 in the PR description. You can merge this pull request into a Git repository by running: $ git pull https://github.com/jeking3/thrift defect/THRIFT-3038 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/thrift/pull/1129.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #1129 ---- commit 356be9a548a7fcd6bc8da9473b822c87bec71315 Author: James E. King, III <jim.k...@simplivity.com> Date: 2016-11-13T04:55:50Z THRIFT-3038 clean up remaining volatile issues; remove unused NoStarveReadWriteMutex class ---- > Use of volatile in cpp library > ------------------------------ > > Key: THRIFT-3038 > URL: https://issues.apache.org/jira/browse/THRIFT-3038 > Project: Thrift > Issue Type: Bug > Components: C++ - Library > Affects Versions: 0.9.2, 0.9.3, 0.10.0 > Reporter: Adriaan Schmidt > Assignee: James E. King, III > Priority: Minor > > In the cpp library there are several member variables which are declared > volatile, I believe with the intention of providing some sort of > thread-safety. > While volatile can be used in this way in Java and C#, in C++ it cannot! It > does not provide any guarantees with regard to instruction (re-)ordering, > i.e. there are no implied memory barriers like you would get by using > explicit locking or atomic variables. > This means that all uses of volatile should be examined, the volatile > qualifier should be removed and replaced by proper synchronization. > The affected member variables are: > # (2nd PR: removed this class - not used) > NoStarveReadWriteMutex::writerWaiting_ > Unprotected read access in acquireRead(). Data race can be seen by running > the unit test with Helgrind. > # (already fixed) TFileTransport::forceFlush_ > Always accessed while holding mutex_. In this case, the volatile can just be > removed. > # (2nd pr: removed volatile) TFileTransport::closing_ > Sometimes accessed while holding mutex_ (in combination with the notEmpty_ > Monitor), > but, e.g., enqueueEvent reads closing_ without any synchronization. > # (already fixed) TThreadPoolServer::stop_, TThreadedServer::stop_ > Accessed (read and written) without synchronization. These would probably be > fine using an atomic data type. Or, use explicit locking or signaling. > # (already fixed) TThreadPoolServer::timeout_, > TThreadPoolServer::taskExpiration_ > Should probably use a lock. > # (2nd pr: removed volatile) Mutex.cpp has mutexProfilingCounter as static > variable. This probably doesn’t break anything, but still the volatile serves > no real purpose. > While some of the fixes are probably simple, in general I think someone with > better knowledge of the code should have a look at this. > Items marked (already fixed) were addressed in > https://github.com/apache/thrift/pull/981 in 0.10.0. -- This message was sent by Atlassian JIRA (v6.3.4#6332)