C++ Can an atomic variable be declared inside a structure to protect those members?

Question

Have a structure declared and defined in shared file.

Both threads create by the Windows API CreateThread() have visibility to the instance of it:

struct info
{
    std::atomic<bool> inUse; 
    string name;

};
info userStruct; //this guy shared between two threads

Thread 1 continually locking/unlocking to write to member in structure (same value for test):

    while (1)
    {
        userStruct.inUse = true;
        userStruct.name= "TEST";
        userStruct.inUse = false;
    }

Thread 2 just reading and printing, only if it happens to catch it unlocked

    while (1)
    {
        while (! userStruct.inUse.load() )
        {
            printf("** %d, %s\n\n", userStruct.inUse.load(), userStruct.name.c_str());
            Sleep(500); //slower reading
        }

        printf("In Use!\n");
    }

Expect to see a lot of:

"In Use!"

and every once if it gets in, when unlocked:

"0, TEST"

..and it does.

But also seeing:

"1, TEST"

If the atomic bool is 1 I do NOT expect to ever see that.

What am I doing wrong?

you are performing 2 distict load on the atomic variable to check then output. the value can change between the loads — Tyker, Nov 18 '18 at 22:59
@tyker: ok I see, so would locking it again before the print and unlocing after the print be a correct way to use it? Thank You. — P.S., Nov 18 '18 at 23:03
@P.S. https://en.cppreference.com/w/cpp/atomic/atomic_compare_exchange is way to go. It compares and updates atomic variables atomically. — Quimby, Nov 18 '18 at 23:04
Sorry, wrong link. Use the member method of atomic<>: https://en.cppreference.com/w/cpp/atomic/atomic/compare_exchange — Quimby, Nov 18 '18 at 23:07
@quimby: could you please provide an example given this example above? — P.S., Nov 18 '18 at 23:08
you nothing locking/unlocked here (unclear what you mean at all). the `name` and `inUse` independent. no any synchronization between threads. and for what atomic here ? — RbMm, Nov 18 '18 at 23:34

Christophe · Accepted Answer · 2018-11-19T00:23:51.950

6

Your code is not thread safe. The atomic is atomic. But the if statement isn't !

What happens:

Thread 1                                Thread 2               Comment 

while (! userStruct.inUse.load() )                             ---> InUse is false 
==> continues loop 
                                        inUse = true           
==> continues loop already started
printf(...)

In the worst case you could have UB due to a data race (one thread 2 modifies the string, and thread 1 reads the string during the modification).

Solution:

Since you intend to use your atomic as a lock, just use a real lock designed for this kind of synchronisation, using a std::mutex with a std::lock_guard.

For example:

struct info
{
    std::mutex access; 
    string name;
};

The first thread would then be:

while (1)
{
    std::lock_guard<std::mutex> lock(userStruct.access); // protected until next iteration
    userStruct.name= "TEST";
}

The second thread could then attempt to gain access to mutex in a non-blocking fashion:

while (1)
{
    {  //  trying to lock the mutex
        std::unique_lock<std::mutex> lock(userStruct.access, std::try_to_lock);
        if(!lock.owns_lock()){   // if not successful do something else
            std::cout << "No lock" <<std::endl; 
        }
        else                     // if lock was successfull
        {
            std::cout << "Got access:" << userStruct.name <<std::endl;
        }
    } // at this stage, the lock is released.
    std::this_thread::sleep_for(std::chrono::milliseconds(500));
}

Online demo

edited Nov 19 '18 at 00:23

answered Nov 18 '18 at 23:08

Christophe

68,716
7
72
138

ahh, now I see it...but not sure if more locking in the other thread would be the correct way to use this? – P.S. Nov 18 '18 at 23:10
1

@P.S. I think you are confusing atomic operation with locking, `atomic` from the STL only provides uninterrupted modification to a trivial type ( in your case a bool ) this does not make anything thread-safe. I think what you are looking for is a proper locking object which would be a [mutex](https://en.cppreference.com/w/cpp/thread/mutex) in the example you can see the usage along with `lock_gaurd` – Chris Mc Nov 18 '18 at 23:15
1

@ChrisMc exactly lock_guard would be the solution for this use case. – Christophe Nov 18 '18 at 23:17
@christophe: Thanks...few questions: In thread 1 the comment says "// protected until next iteration"...is that because it goes out of scope back in the while() statement and destroys the object? Is the member "userStruct.name" protected beucase how we are interpreting and specifically using the lock? or how does "lock(userStruct.access);" protect the other member? Thank you. – P.S. Nov 20 '18 at 00:24
1

@P.S. Exactly! The lock_guard locks the mutex. At the end of the loop it goes out of scope, gets destroyed and therfore releases the mutex. THe protection is given by the mutex and the **convention** that you first aquire the mutex before using the other members (or any other resource or object in your code). The lock doesn't prevent against rogue access that is made without repect of the convention. – Christophe Nov 20 '18 at 07:59

Tyker · Answer 2 · 2018-11-19T00:09:53.370

5

you are performing 2 distict load on the atomic variable to check then output. the value can change between the loads. also you have a data race on your string variable.

you can fixe it easily by using std::atomic_flag or a mutex

struct info
{
    std::atomic_flag inUse;
    std::string name;

};

//writer
while (1)
{
    if (!userStruct.inUse.test_and_set()) {
        userStruct.name= "TEST";
        userStruct.inUse.clear();
    }
}

//reader
while (1)
{
    if (!userStruct.inUse.test_and_set())
    {
        printf("** %s\n\n", userStruct.name.c_str());
        userStruct.inUse.clear();
    }
    printf("In Use!\n");
}

you can't check the value in atomic_flag because it is almost always a bad idea to check the value of a lock because the value can change before you take your action.

edited Nov 19 '18 at 00:09

answered Nov 18 '18 at 23:25

Tyker

2,971
9
21

1

I would probably use a `mutex` or even a `shared_mutex`, in the off chance that the code inside the protected section throws an exception. A mutex lock will unlock automatically when it goes out of scope. An `atomic_flag` does not clear automatically, you would need to make a custom RAII wrapper for that. – Remy Lebeau Nov 18 '18 at 23:57
1

code is wrong anyway. the reader and writer can at once be inside `if` - first reader enter to if and set `inUse` to true, then writer enter to `if` to. code nothing synchronize and wrong – RbMm Nov 19 '18 at 00:06
1

The `if` statement in the write needs to be inverted; you want to enter that block if the `inUse` flag is false – LWimsey Nov 19 '18 at 00:08
1

@LWimsey i fixed it – Tyker Nov 19 '18 at 00:13
1

in this case yes, correct after fix. the code for enter critical region must be symmetric for both threads. also for highlight main concept - better use `if (!userStruct.inUse.test_and_set(memory_order_acquire)) { ... userStruct.inUse.clear(memory_order_release); }` – RbMm Nov 19 '18 at 00:17

Quimby · Answer 3 · 2018-11-18T23:43:41.670

As Tyker pointed out in the comment, you have a race condition.( No need for inner while if its in infinite loop anyway.)

if (! userStruct.inUse.load() )
{
    //inUse might change in the middle printf
    printf("** %d, %s\n\n", userStruct.inUse.load(), userStruct.name.c_str());
    Sleep(500); //slower reading
}
else
   printf("In Use!\n");

Solution is to "lock" the reading, but simply doing the following is still not safe:

if (! userStruct.inUse.load() ) //#1
{
    //inUse might already be true here, so we didn't lock quickly enough. 
    userStruct.inUse=true; //#2
    printf("** %d, %s\n\n", userStruct.inUse.load(), userStruct.name.c_str());
    userStruct.inUse=false;
    Sleep(500); //slower reading
}

So, truly safe code is to fuse #1, #2 together:

bool f=false;
//Returns true if inUse==f and sets it to true
if(userStruct.inUse.compare_exchange_strong(f,true))
{
    printf("** %d, %s\n\n", userStruct.inUse.load(), userStruct.name.c_str());
    userStruct.inUse=false;
    Sleep(500); //slower reading
}

what sense of `userStruct.inUse.compare_exchange_strong(f,true)` ? you anyway got random value from `userStruct.inUse.load()` after this — RbMm, Nov 18 '18 at 23:42
Quimby, Although it's not wrong, technically you don't need a compare_exchange for a spinlock. See [this question](https://stackoverflow.com/questions/53280739/does-acquiring-a-spinlock-require-compare-and-swap-or-is-swap-enough) — LWimsey, Nov 19 '18 at 00:32

C++ Can an atomic variable be declared inside a structure to protect those members?

3 Answers3

Solution: