CODEWITHSUNDEEP
c++windowsmultithreadingatomic
P.S.
P.S.

Reputation: 384

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

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?

Upvotes: 3

Views: 1443

Answers (3)

Christophe
Christophe

Reputation: 73366

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

Upvotes: 6

Tyker
Tyker

Reputation: 3047

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.

Upvotes: 5

Quimby
Quimby

Reputation: 19113

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
}

Upvotes: 2

Related Questions