1

I've a function that accesses(reads and writes to) a std::atomic<bool> variable. I'm trying to understand the order of execution of instructions so as to decide whether atomic will suffice or will I've to use mutexes here. The function is given below -

// somewhere member var 'executing' is defined as std::atomic<bool>`

int A::something(){
    
    int result = 0;
    // my intention is only one thread should enter next block
    // others should just return 0
    if(!executing){
        executing = true;

        ...
        // do some really long processing
        ...
       
        result    = processed;
        executing = false;
    }
    
    return result;
}

I've read this page on cppreference which mentions -

Each instantiation and full specialization of the std::atomic template defines an atomic type. If one thread writes to an atomic object while another thread reads from it, the behavior is well-defined (see memory model for details on data races)

and on Memory model page the following is mentioned -

When an evaluation of an expression writes to a memory location and another evaluation reads or modifies the same memory location, the expressions are said to conflict. A program that has two conflicting evaluations has a data race unless either

  • both conflicting evaluations are atomic operations (see std::atomic)

  • one of the conflicting evaluations happens-before another (see std::memory_order)

If a data race occurs, the behavior of the program is undefined.

and slight below it reads -

When a thread reads a value from a memory location, it may see the initial value, the value written in the same thread, or the value written in another thread. See std::memory_order for details on the order in which writes made from threads become visible to other threads.


This is slightly confusing to me, which one of above 3 statements are actually happening here?

When I perform if(!executing){ is this instruction an atomic instruction here? and more important - is it guaranteed that no other thread will enter that if loop if one two threads will enter that if body since first one will set executing to true?

And if something's wrong with the mentioned code, how should I rewrite it so that it reflects original intention..

Community
  • 1
  • 1
Abhinav Gauniyal
  • 7,034
  • 7
  • 50
  • 93
  • 4
    `if(!executing){ executing = true;` another thread can get in between these 2 statements. Have a look at `std::atomic<>.compare_exchange_XXX` – Richard Critten Oct 05 '16 at 09:30
  • Your use case is strange. Why fire multiple threads if only one of them can `do some really long processing` at a time? – Mat Oct 05 '16 at 09:32
  • @Mat user might click on the button several times and I don't want to disable button for the time being, if the result hasn't been computed yet the return value 0 will be checked and user will be notified to wait.. is there a better way to do this? – Abhinav Gauniyal Oct 05 '16 at 09:34
  • 2
    Yes, disable the button, or change its state so that it tells the user what's going on. – Mat Oct 05 '16 at 09:36
  • Agreed with @Mat. A disabled button signals that the user actually hit it, the program noticed it and it's working on it. – Baum mit Augen Oct 05 '16 at 09:38
  • 1
    @Mat I explicitly mentioned I cannot disable the button :/ even if I would some user will fire the api himself and will do whatever the button was doing at it's first place. This isn't a native application where I can hide my api. – Abhinav Gauniyal Oct 05 '16 at 09:40
  • 2
    No, you said you didn't want to, not that you couldn't, and you said nothing about an API. If you want advice about how to do whatever it is you're trying to achieve, you'll need to provide more accurate details about what that is exactly. (In your question.) – Mat Oct 05 '16 at 09:43
  • 3
    @Mat my question isn't about hiding or showing button or any other interface to the user. – Abhinav Gauniyal Oct 05 '16 at 09:48
  • 2
    Got that. But what is it exactly then? The code you posted is broken in terms of synchronization. If you want to know how to do something correctly, you need to describe that thing correctly too, especially when threads are involved. These things are hard, and missing details or misunderstood requirements usually lead to pain and tears in production. – Mat Oct 05 '16 at 09:50
  • @Mat I've written it as comment inside the code. `// my intention is only one thread should enter next block` `// others should just return 0`. I apologize if it wasn't clear, I just want other incoming functions to just return 0 without doing that computation part, otherwise they'd be all doing same thing and return multiple times. And I'm the one who has recieved this code from someone else to maintain, the tests are passing but I somehow caught it and was suspicious that this might be wrong. – Abhinav Gauniyal Oct 05 '16 at 09:52

1 Answers1

5

If I understand correctly, you are trying to ensure that only one thread will ever execute a stretch of code at the same time. This is exactly what a mutex does. Since you mentioned that you don't want threads to block if the mutex is not available, you probably want to take a look at the try_lock() method of std::mutex. See the documentation of std::mutex.

Now to why your code does not work as intended: Simplifying a little, std::atomic guarantees that there will be no data races when accessing the variable concurrently. I.e. there is a well defined read-write order. This doesn't suffice for what you are trying to do. Just imagine the if branch:

if(!executing) {
   executing = true;

Remember, only the read-write operations on executing are atomic. This leaves at least the negation ! and the if itself unsynchronized. With two threads, the execution order could be like this:

  1. Thread 1 reads executing (atomically), value is false
  2. Thread 1 negates the value read from executing, value = true
  3. Thread 1 evaluates the condition and enters the branch
  4. Thread 2 reads executing (atomically), value is false
  5. Thread 1 set executing to true
  6. Thread 2 negates the value, which was read as false and is now true again
  7. Thread 2 enters the branch...

Now both threads have entered the branch.

I would suggest something along these lines:

std::mutex myMutex;

int A::something(){

    int result = 0;
    // my intention is only one thread should enter next block
    // others should just return 0
    if(myMutex.try_lock()){

        ...
        // do some really long processing
        ...

        result    = processed;
        myMutex.unlock();
    }

    return result;
}
Pete Becker
  • 74,985
  • 8
  • 76
  • 165
Drag-On
  • 362
  • 2
  • 9
  • `std::atomic<>.compare_exchange_XXX` will also do what the OP requires. No need for a full mutex. – Richard Critten Oct 05 '16 at 10:47
  • Whilst mutex is definitely the right answer to this, you can achieve similar functionality to this example by using compare_exchange_strong which will atomically read and conditionally update the variable in a single atomic operation – Mike Vine Oct 05 '16 at 10:48
  • @RichardCritten. Yes it can work but that doesn't mean its the correct answer to the implied question. – Mike Vine Oct 05 '16 at 10:49
  • This really seems a good answer but I ain't sure about this part - `This function is allowed to fail spuriously and return false even if the mutex is not currently locked by any other thread.` – Abhinav Gauniyal Oct 05 '16 at 13:49
  • 1
    It's true that try_lock() can fail spuriously in rare cases. This is in the standard to allow some very fast implementations. However, if you call it multiple times from various threads, it is safe to assume that one of them will acquire the lock. For more details, see for example [here](http://stackoverflow.com/questions/28005135/mistake-in-cpp-specification-about-mutex-try-lock) and [here](http://stackoverflow.com/questions/33907999/stdtimed-mutextry-lock-fail-spuriously) – Drag-On Oct 05 '16 at 14:47