[jira] [Commented] (THRIFT-3038) Use of volatile in cpp library

["ASF GitHub Bot (JIRA)" <ji...@apache.org>]

    [ 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 <ji...@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)