Threadsafety of atomic int collection

Question

I have the following class wrapping a vector of atomic ints ( std::vector< std::atomic_int > )

The vector is sized correctly at object construction and doesn't change size. There are the usual accessors and mutators for getting, setting the atomic ints, but no guards / mutex.

class MyList
{
    std::vector< std::atomic_int > collection_;

    static MyList myList_;

public:

    MyList() : collection_( MAX_SIZE, 0 ) {}

    static MyList& getMyList() { return myList_; }

    void set( size_t idx, int val )
    {
        collection_[idx].store( val, std::memory_order_relaxed );
    }

    int get( size_t idx ) const
    {
        return collection_[idx].load( std::memory_order_relaxed );
    }
};

I'm minded to suspect that this might not be thread-safe (it is currently running in a single-threaded model without problem), but would appreciate any views. My main concern is with the thread-safety of the unguarded collection, I suppose, rather than the elements of it.

If the container itself is treated as an entity (elements relate to other elements), it is not thread safe. — , Sep 08 '15 at 15:53
For instance, could I guarantee the integrity of the vector across different threads running on different cores (assuming it may be cached per core)? — Component 10, Sep 08 '15 at 16:09

score 3 · Accepted Answer · edited May 23 '17 at 11:44

First, it's important to note that you can't have a vector of atomic ints without some shenanigans.

Ignoring that, according to [container.requirements.dataraces] if you only access the vector to modify its content, then this seems to be thread-safe.

For purposes of avoiding data races (17.6.5.9), implementations shall consider the following functions to be const: begin, end, rbegin, rend, front, back, data, find, lower_bound, upper_bound, equal_range, at and, except in associative or unordered associative containers, operator[].

Notwithstanding (17.6.5.9), implementations are required to avoid data races when the contents of the con- tained object in different elements in the same container, excepting vector<bool>, are modified concurrently.

The wording isn't extremely clear on whether operator[] could possibly be non thread-safe in this case, but in practice no reasonable implementation should violate this.

If you want more guarantees, and since the vector doesn't change size, you could replace the vector<T> with a unique_ptr<T[]>, which is trivially thread-safe in this case.
Additionally, you should use a memory order that guarantees safe synchronization and ordering (unless you have a very good reason), instead of memory_order_relaxed. Not specifying a memory order at all, or using a memory_order_acquire/memory_order_release pair does this.
This results in the following very similar code:

class MyList
{
    std::unique_ptr< std::atomic_int[] > collection_;

    static MyList myList_;

public:

    MyList() : collection_( new atomic_int[MAX_SIZE] ) {}

    static MyList& getMyList() { return myList_; }

    void set( size_t idx, int val )
    {
        collection_[idx].store( val, std::memory_order_release );
    }

    int get( size_t idx ) const
    {
        return collection_[idx].load( std::memory_order_acquire );
    }
};

I don't know if it applies here, but there are non-default options for atomic operations, one of which is memory_order_relaxed, which would not be thread safe. Any default (non-specified) option should result in thread safe atomic operations. — rcgldr, Sep 08 '15 at 21:40
@rcgldr You're absolutely right, I assumed OP had a good reason to explicitly use memory_order_relaxed here, but it does seem somewhat sketchy. Using either the default or a memory_order_release/acquire pair would actually enforce synchronization. — tux3, Sep 08 '15 at 21:48
@rcgldr What do you mean? Atomic operations are always thread safe, regardless of the options. [This](http://stackoverflow.com/a/30691672/721269) is not precisely on point, but hopefully close enough. — David Schwartz, Sep 08 '15 at 22:03
@tux3 What's sketchy about using `memory_order_relaxed`? If he just needs thread-safe atomic operations and doesn't need ordering with respect to non-atomic operations, he should specify `memory_order_relaxed`. That appears to be all he needs. — David Schwartz, Sep 08 '15 at 22:04
@DavidSchwartz If this really is what OP needs, `memory_order_relaxed` is fine. But I can see how a thread safe container whose getter/setters are not synchronizing could be surprising, so I think acquire/release is a safe choice here. — tux3, Sep 08 '15 at 22:13
@tux3 It's definitely safer. But if you just need to update values and get the latest values, `memory_order_relaxed` is perfect. — David Schwartz, Sep 08 '15 at 22:15

Threadsafety of atomic int collection

1 Answers1