[
https://issues.apache.org/jira/browse/THRIFT-3038?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15661236#comment-15661236
]
ASF GitHub Bot commented on THRIFT-3038:
----------------------------------------
Github user nsuke commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1129#discussion_r87707817
--- Diff: lib/cpp/src/thrift/concurrency/Mutex.cpp ---
@@ -37,11 +37,10 @@ namespace concurrency {
#ifndef THRIFT_NO_CONTENTION_PROFILING
+static sig_atomic_t mutexProfilingCounter = 0;
static sig_atomic_t mutexProfilingSampleRate = 0;
--- End diff --
Are we dealing with signal here at all ?
Otherwise, changing these to int/int32_t would be a good idea.
mutexProfilingCounter can arguably be atomic if we want to avoid data race
(technically UB) in exchange for some performance penalty.
> 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)
