Acquiring lock by checking against a condition and rechecking it

Question

Is something like this valid:

std::vector<std::vector<int>> data;
std::shared_mutex m;
...

void Resize() {
    // AreAllVectorsEmpty: std::all_of(data.begin(), data.end(), [](auto& v) { return v.empty(); }
    if (!AreAllVectorsEmpty()) {
        m.lock();
        if (!AreAllVectorsEmpty()) {
            data.resize(new_size);
        }
        m.unlock();
    }
}

I am checking AreAllVectosEmpty() and then if condition succeeds, then taking lock and then again check for the same condition whether to do the resize.

Would this be thread safe? Resize is only called by one thread, but other threads manipulate elements of data.

Is it a requirement that AreAllVectorsEmpty have a memory fence or acquire semantics?

Edit: Other threads would ofcourse block when m.lock is acquired by Resize.

Edit: Let's also assume new_size is large enough that reallocation happens.

Edit: Update code for shared_mutex.

Edit: AreAllVectorsEmtpy is iterating over the data vector. Nobody else modifies data vector, but data[0], data[1] etc are modified by other threads. My assumption is since data[0]'s size variable is inside the vector and is a simple integer, it is safe to access data[0].size(), data[1].size() etc... in the Resize thread. AreAllVectorsEmpty is iterating over data and checking vector.empty().

if this function does not modify shared resource, it is thread-safe — mangusta, May 29 '18 at 06:39
@JesperJuhl let's say this way, it's all about immediate and eventual consistency. sometimes eventual consistency could be tolerated, sometimes not. it depends — mangusta, May 29 '18 at 06:44
Or std::lock_guard (for [comparison](https://stackoverflow.com/a/20516876/1312382)). — Aconcagua, May 29 '18 at 07:00
This is called “double checked locking”. In general it doesn’t work. — Pete Becker, May 29 '18 at 07:40

Patrick · Answer 1 · 2018-05-29T06:49:07.360

3

I would use a shared_mutex and use:

a shared lock in all threads that just read the vector (while reading the vector)
a unique lock in this thread when resizing the vector

I think first checking for the size, then resizing it, is safe, provided that this is the only thread that modifies the contents of the vector.

A lock automatically implies a memory barrier, otherwise the lock would not make much sense.

edited May 29 '18 at 06:49

answered May 29 '18 at 06:45

Patrick

23,217
12
67
130

I am using `shared_mutex`. For the simplicity of the example, I changed it to `mutex`. Anyway, yes `Resize` is called by one thread. – themagicalyang May 29 '18 at 06:47

score 3 · Answer 2 · answered May 29 '18 at 06:52

3

The answer depends entirely on how AreAllVectorsEmpty is implemented.

If it just checks a flag that can be set atomically, then yes, it is safe. If it iterates over the vector you intend to change (or other commonly used containers), then no, it is not safe (what happens to iterators, if the vector does re-allocation internally???).

If doing the latter, you need a read/write lock mechanism, have a look at shared mutexes.

You'd then acquire the shared lock before checking, and in case of modification, the exclusive lock.

Be aware that if areAllVectorsEmpty uses some independent data structure (other than the mentioned atomic flag), you might have to protect this one with a separate mutex as well.

answered May 29 '18 at 06:52

Aconcagua

24,880
4
34
59

It is iterating over the data vector. Nobody else modifies data vector, but data[0], data[1] etc are modified by other threads. My assumption is since data[0]'s size variable is inside the vector and is a simple integer, it is safe to access data[0].size(), data[1].size() etc... in the `Resize` thread. `AreAllVectorsEmpty` is iterating over `data` and checking `vector.empty()`. – themagicalyang May 29 '18 at 06:59
Well, then: If you do not protect the iteration over `data` itself, any thread might acquire the lock *while* another thread *still* is iterating over - and then resizing might invalidate latter's iterators! Additionally: be aware that `std::vector::size` is often implemented as `end() - begin()`, and `std::vector::empty` again as `size() == 0`. You cannot rely on them being thread-safe, thus you might even have to protect the internal vectors against concurrent read/write conflicts appropriately! – Aconcagua May 29 '18 at 07:14
Resizing on the data vector is only done from one thread, the thread that already calls Resize. also mentioned on the question – themagicalyang May 29 '18 at 07:19
Good general advice: Don't ever rely on assumptions, you can fail badly if they are not correct! – Aconcagua May 29 '18 at 07:19
Some example: 4 elements in the vector, thread A starts iterating and reaches elements 0 and 1. Then thread B interrupts A, and as reading is not protected, can acquire the lock. Sure, it is the only thread to do the modification now, but thread A is right in the middle of its iteration! What will happen to its iterators? – Aconcagua May 29 '18 at 07:22

score 2 · Answer 3 · answered May 29 '18 at 06:53

The standard does not seem to request that this works, compare http://en.cppreference.com/w/cpp/container#Thread_safety. If it works with your specific compiler and STL? You'll need to look into the sources. But I would not rely on it.

This brings me to the question: why do you want to do it? For performance reasons? Have you measured performance? Is it really a measurable performance hit when you lock before calling AreAllVectorsEmpty?

BTW, please don't directly lock the mutex, please use a std::lock_guard.

score 0 · Answer 4 · answered May 29 '18 at 07:11

// AreAllVectorsEmpty: std::all_of(data.begin(), data.end(), [](auto& v) { return v.empty(); }

you are accessing internals of the inner vectors (calling empty) and the same time another thread could insert some elements into one of the inner vectors -> data race

Acquiring lock by checking against a condition and rechecking it

4 Answers4