[
https://issues.apache.org/jira/browse/THRIFT-3038?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
James E. King, III updated THRIFT-3038:
---------------------------------------
Description:
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.
was:
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.
# (already fixed) 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.
# (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.
# 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.
> 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)
