std::atomic - comaprison with multiple values

[Christopher Pisz]

It seems to me there is a subtle problem with the following code:



#include <atomic>

class State
{
public:
enum ServiceState
{
STOPPING = 0
, STOPPED
, STARTING
, STARTED
};

State(State::ServiceState initialState);

ServiceState GetState() const;
void SetState(ServiceState state);

private:

std::atomic<ServiceState> m_state;
};

State::State(State::ServiceState initialState)
:
, m_state(initialState)
{
}

//----------------------------------------------------------------------------------------
State::ServiceState State::GetState() const
{
return m_state;
}

//----------------------------------------------------------------------------------------
void State::SetState(ServiceState state)
{
// TODO - Log the state change

m_state = state;
}



int main()
{
State state(State::STARTED);

// TODO - State can chnange between OR comparisons
// The encapsulation of the atomic object destroyed the guarentee
// that a comparison of an atomic with a single value would provide
//
// On the otherhand, I am not sure how you would get the same behavior
// from the atomic that you would get with
//
// enum State{STOPPING = 0, STOPPED, STARTING, STARTED};
//
// bool running = false;
// {
// std::lock_guard<std::mutex> lock();
// running = (state == State::STARTED || state == State::STARTING)
// }
//
// if( running ) { //do stuff }
//
if (state.GetState() == State::STARTED || state.GetState() == State::STARTING)
{
// Do stuff
}

return 0;
}



As I describe in the todo before the comparison. I am thinking about wta happens when we compare with multiple values in ORed like that. One comparison occurs at a time, so we re no longer really atomic.

Is there a way to get fix and still encapsulate atomic, or do we need to go back to using a lock and mutex when comparing multiple values like that?

[Scott Lurndal]

if (state.is_state(State::STARTED|State::STARTING)) {
...
}


bool State::is_state(int flags) const { return (int)m_state.load() & flags; }

[Marcel Mueller]

On 09.03.17 18.27, Christopher Pisz wrote:
> std::atomic<ServiceState> m_state;
[...]

> //----------------------------------------------------------------------------------------
> State::ServiceState State::GetState() const
> {
> return m_state;
> }
>
> //----------------------------------------------------------------------------------------
> void State::SetState(ServiceState state)
> {
> // TODO - Log the state change
>
> m_state = state;
> }

std::atomic is of no particular use in your code since anyone can do any
state change at any time.

Normally a state machine should control /valid/ state changes.

> int main()
> {
> State state(State::STARTED);
>
> // TODO - State can chnange between OR comparisons
> // The encapsulation of the atomic object destroyed the guarentee
> // that a comparison of an atomic with a single value would provide

Agreed. But the result is undefined anyway since the state can change
/before/ the condition as well.

> // On the otherhand, I am not sure how you would get the same behavior
> // from the atomic that you would get with
> //
> // enum State{STOPPING = 0, STOPPED, STARTING, STARTED};
> //
> // bool running = false;
> // {
> // std::lock_guard<std::mutex> lock();
> // running = (state == State::STARTED || state == State::STARTING)
> // }

#1:
State l_state = state;
running = l_state == State::STARTED || l_state == State::STARTING;

#2
running = state >= State::STARTING;

> // if( running ) { //do stuff }
> //
> if (state.GetState() == State::STARTED || state.GetState() == State::STARTING)
> {
> // Do stuff
> }
>

> As I describe in the todo before the comparison. I am thinking about wta happens when we compare with multiple values in ORed like that. One comparison occurs at a time, so we re no longer really atomic.

That's true. But it does not make any difference in your code because at
"Do stuff" state is undefined anyway since it can change at /any/ time
not just between the comparisons.

> Is there a way to get fix and still encapsulate atomic, or do we need to go back to using a lock and mutex when comparing multiple values like that?

The comparison is not your problem. The code at DoStuff probably need to
be synchronized with state changes.


Marcel

[Christopher Pisz]

For clarity let me change the code using a mutex and lock that we are comparing against. The original way in comments using a mutex really has the same problem. In the end, I want to ensure the state does not change while comparing state against multiple possible states.

/* Alternate way using mutex*/
std::unique_lock<std::mutex> lock(g_mutex);
while (g_state == ServiceState::STARTED || g_state == ServiceState::STARTING)
{
lock.unlock();

// Do Stuff

lock.lock();
}

lock.unlock();

[Marcel Mueller]

On 09.03.17 20.01, Christopher Pisz wrote:
> For clarity let me change the code using a mutex and lock that we are comparing against. The original way in comments using a mutex really has the same problem. In the end, I want to ensure the state does not change while comparing state against multiple possible states.
>
> /* Alternate way using mutex*/
> std::unique_lock<std::mutex> lock(g_mutex);
> while (g_state == ServiceState::STARTED || g_state == ServiceState::STARTING)
> {
> lock.unlock();
>
> // Do Stuff
>
> lock.lock();
> }
>
> lock.unlock();

The lock still makes no sense here.


Marcel

[Christopher Pisz]

Can you elaborate on why this makes no sense?
The intention is to run the loop until state is not "started" or "starting"

If one thread reads and another write to a variable, we lock it.
Why does the lock make no sense?

Are you claiming that an integer type is atomic by default?

[Christopher Pisz]

On Thursday, March 9, 2017 at 12:25:23 PM UTC-6, Marcel Mueller wrote:
SNIP

> That's true. But it does not make any difference in your code because at
> "Do stuff" state is undefined anyway since it can change at /any/ time
> not just between the comparisons.
>
>
> > Is there a way to get fix and still encapsulate atomic, or do we need to go back to using a lock and mutex when comparing multiple values like that?
>
> The comparison is not your problem. The code at DoStuff probably need to
> be synchronized with state changes.
>
>
> Marcel

I really don't care if the state changes while I am "doing stuff."
I only care that the state was exactly what we checked for in the condition when we checked it.

[Chris Vine]

In practice on any architecture you could be interested in, ints (but
not necessarily all integer types) are atomic (in the
std::memory_order_relaxed sense).

However your latest question above is irrelevant to your original
question, wherein you correctly pointed out that your mutex locking
makes the ORed comparison atomic, whereas just substituting atomic
variables, of whatever memory order, without some other synchronization
mechanism between them such as "test and loop" will not.

This raises a number of points which would need to be answered to solve
your first question. In particular, why does it make a difference in
your use case that the ORed enumeration comparison should be atomic
when it does not matter that the dependent operations (those covered by
your "Do Stuff") are not also atomic with respect to the enumeration
comparisons? Possibly your code was implementing some kind of test and
rollback before committing changes but that is not immediately obvious,
and in that case does the first comparison need to be atomic at all?

I think that was Marcel's point.