[ https://issues.apache.org/jira/browse/THRIFT-3038?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14561759#comment-14561759 ]
Tomasz Kupczyk edited comment on THRIFT-3038 at 5/27/15 9:20 PM: ----------------------------------------------------------------- For issue number 1. it looks like it would be enough to make `writerWaiting_` an atomic integer/bool variable. That would solve the synchronization problem and the data race. I haven't noticed though usage of any atomic types in thrift cpp files. 'C++11' supports atomic types and as far as I can see we compile with '-std=c++11' option. It can have an impact on performance as as far as I'm aware when using atomics memory barriers may are involved. It's hard to say what will be the impact. I can provide a patch for that and try do some performance testing. Any objections/suggestions? was (Author: flojdek): For issue number 1. it looks like it would be enough to make `writerWaiting_` an atomic integer/bool variable. That would solve the synchronization problem and the data race. I haven't noticed though usage of any atomic types in thrift cpp files. C++11 supports atomic types and as far as I can see we compile with -std=c++11 option. It can have an impact on performance as as far as I'm aware when using atomics memory barriers may are involved. It's hard to say what will be the impact. I can provide a patch for that and try do some performance testing. Any objections/suggestions? > 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 > Reporter: Adriaan Schmidt > > 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: > # NoStarveReadWriteMutex::writerWaiting_ > Unprotected read access in acquireRead(). Data race can be seen by running > the unit test with Helgrind. > # TFileTransport::forceFlush_ > Always accessed while holding mutex_. In this case, the volatile can just be > removed. > # TFileTransport::closing_ > Sometimes accessed while holding mutex_ (in combination with the notEmpty_ > Monitor), > but, e.g., enqueueEvent reads closing_ without any synchronization. > # 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. > # TThreadPoolServer::timeout_, TThreadPoolServer::taskExpiration_ > Should probably use a lock. > # 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. -- This message was sent by Atlassian JIRA (v6.3.4#6332)