Atomic compare exchange. Is it correct?

Question

I want to atomically add 1 to a counter under certain conditions, but I'm not sure if following is correct in a threaded environment:

void UpdateCounterAndLastSessionIfMoreThan60Seconds() const {
    auto currentTime = timeProvider->GetCurrentTime();
    auto currentLastSession = lastSession.load();
    bool shouldIncrement = (currentTime - currentLastSession >= 1 * 60);

    if (shouldIncrement) {
        auto isUpdate = lastSession.compare_exchange_strong(currentLastSession, currentTime);
        if (isUpdate) 
            changes.fetch_add(1);
    }
}

private:
    std::shared_ptr<Time> timeProvider;
    mutable std::atomic<time_t> lastSession;
    mutable std::atomic<uint32_t> changes;

I don't want to increment changes multiple times if 2 threads simultaneously evaluate to shouldIncrement = true and isUpdate = true also (only one should increment changes in that case)

Answer 1

It is at least a bit iffy, as following scenario will show:

First thread 1 does these:

auto currentTime = timeProvider->GetCurrentTime();
auto currentLastSession = lastSession.load();
bool shouldIncrement = (currentTime - currentLastSession >= 1 * 60);

Then thread 2 does the same 3 statements, but so that currentTime is more than it just was for Thread 1.

Then thread 1 continues to update lastSession with it's time, which is less than thread 2's time.

Then thread 2 gets its turn, but fails to update lastSession, because thread 1 changed the value already.

So end result is, lastSession is outdated, because thread 2 failed to update it to the latest value. This might not matter in all cases, situation might be fixed very soon after, but it's one ugly corner which might break some assumptions somewhere, if not with current code then after some later changes.

---

Another thing to note is, lastSession and chnages are not atomically in sync. Other threads occasionally see changed lastSession with changes counter still not incremeted for that change. Again this is something which might not matter, but it's easy to forget something like this and accidentally code something which assumes they are in sync.

---

I'm not immediately sure if you can make this 100% safe with just atomics. Wrap it in a mutex instead.

Answer 2

I'm no C++ expert, but it looks to me like you've got a race condition between the evaluation of "isUpdate" and the call to "fetch_add(1)".

So I think the answer to your question "Is this thread safe?" is "No, it is not".