[
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:23 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. Cpp11 supports atomic types and as far as I can see we compile with
Cpp11 support. It can have an impact on performance as as far as I'm aware when
using atomics memory barriers 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. Cpp11 supports atomic types and as far as I can see we compile with
Cpp11 support. 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)
