Creating a thread safe atomic counter

Question

I have a specific requirement in one of the my projects, that is keeping "count" of certain operations and eventually "reading" + "resetting" these counters periodically (eg. 24 hours).

Operation will be:

• Worker threads -> increment counters (randomly)
• Timer thread (eg. 24 hours) -> read count -> do something -> reset counters

The platform I am interested in is Windows, but if this can be cross platform even better. I'm using Visual Studio and target Windows architecture is x64 only.

I am uncertain if the result is "ok" and if my implementation is correct. Frankly, never used much std wrappers and my c++ knowledge is quite limited.

Result is:

12272 Current: 2
12272 After: 0
12272 Current: 18
12272 After: 0
12272 Current: 20
12272 After: 0
12272 Current: 20
12272 After: 0
12272 Current: 20
12272 After: 0

Below is a fully copy/paste reproducible example:

#include <iostream>
#include <chrono>
#include <thread>
#include <Windows.h>

class ThreadSafeCounter final 
{
private:
    std::atomic_uint m_Counter1;
    std::atomic_uint m_Counter2;
    std::atomic_uint m_Counter3;
public:
    ThreadSafeCounter(const ThreadSafeCounter&) = delete;
    ThreadSafeCounter(ThreadSafeCounter&&) = delete;
    ThreadSafeCounter& operator = (const ThreadSafeCounter&) = delete;
    ThreadSafeCounter& operator = (ThreadSafeCounter&&) = delete;

    ThreadSafeCounter() : m_Counter1(0), m_Counter2(0), m_Counter3(0) {}
    ~ThreadSafeCounter() = default;

    std::uint32_t IncCounter1() noexcept 
    {
        m_Counter1.fetch_add(1, std::memory_order_relaxed) + 1;
        return m_Counter1;
    }

    std::uint32_t DecCounter1() noexcept
    {
        m_Counter1.fetch_sub(1, std::memory_order_relaxed) - 1;
        return m_Counter1;
    }

    VOID ClearCounter1() noexcept
    {
        m_Counter1.exchange(0);
    }
};

int main()
{
    static ThreadSafeCounter Threads;

    auto Thread1 = []() {
        while (true)
        {
            auto test = Threads.IncCounter1();
            std::cout << std::this_thread::get_id() << " Threads.IncCounter1() -> " << test << std::endl;

            std::this_thread::sleep_for(std::chrono::seconds(2));
        }
    };

    auto Thread2 = []() {
        while (true)
        {
            auto test = Threads.DecCounter1();
            std::cout << std::this_thread::get_id() << " Threads.DecCounter1() -> " << test << std::endl;

            std::this_thread::sleep_for(std::chrono::seconds(2));
        }
    };

    auto Thread3 = []() {
        while (true)
        {
            Threads.ClearCounter1();
            std::cout << std::this_thread::get_id() << " Threads.ClearCounter1()" << std::endl;

            std::this_thread::sleep_for(std::chrono::seconds(2));
        }
    };

    std::thread th1(Thread1);
    std::thread th2(Thread2);
    std::thread th3(Thread3);

    th1.join();
    th2.join();
    th3.join();
}

I should mention that in my real life project there is no usage of std::thread wrapper, and the threads are created using WinApi functions like CreateThread. The above is just to simulate/test the code.

Please point out to me what is wrong with the above code, what could be improved and if I'm on the right direction at all.

Thank you!

Answer 1

Why are you writing a ThreadSafeCounter class at all?

std::atomic<size_t> is a ThreadSafeCounter. That's the whole point of std::atomic. So you should use it instead. No need for another class. Most atomics have operator++/operator-- specializations, so your main loop could easily be rewritten like this:

    static std::atomic_int ThreadCounter1(0);

    auto Thread1 = []() {
        while (true)
        {
            auto test = ++ThreadCounter1;  // or ThreadCounter1++, whatever you need
            std::cout << std::this_thread::get_id() << " Threads.IncCounter1() -> " << test << std::endl;

            std::this_thread::sleep_for(std::chrono::seconds(2));
        }
    };

    auto Thread2 = []() {
        while (true)
        {
            auto test = --ThreadCounter1;
            std::cout << std::this_thread::get_id() << " Threads.DecCounter1() -> " << test << std::endl;

            std::this_thread::sleep_for(std::chrono::seconds(2));
        }
    };

    auto Thread3 = []() {
        while (true)
        {
/* Note: You could simply "ThreadCounter1 = 0" assign it here.
But exchange not only assigns a new value, it returns the previous value.
*/
            auto ValueAtReset=ThreadCounter1.exchange(0);
            std::cout << std::this_thread::get_id() << " Threads.ClearCounter1() called at value" << ValueAtReset << std::endl;

            std::this_thread::sleep_for(std::chrono::seconds(2));
        }
    };

I forgot to mention a problem with your DecCounter operation. You are using atomic_uint, which cannot handle negative numbers. But there is no guarantee, that your Thread2 will not run (aka decrement the counter) before Thread1. Which means that your counter will wrap.

So you could/should use std::atomic<int> instead. That will give you the correct number of (calls_Thread1 - calls_Thread2). The number will get negative if Thead2 has decremented the value more often than Thread1.

Answer 2

This looks suspicious:

std::uint32_t IncCounter1() noexcept 
{
    m_Counter1.fetch_add(1, std::memory_order_relaxed) + 1;
    return m_Counter1;
}

The + 1 is at the end of that line is effectively a no-op since the code does not assign the result of that expression to anything. Further, you create a race condition by referencing m_Counter1 again on the second line - as there could have easily been a context switch to another thread changing it's value between performing the fetch_add and then referencing the value again for the return result.

I think you want this instead:

std::uint32_t IncCounter1() noexcept 
{
    return m_Counter1.fetch_add(1, std::memory_order_relaxed) + 1;
}

fetch_add will return the previously held value before the increment. So returning that +1 will be the current value of the counter at that moment.

Same issue in DecCounter1. Change that function's implementation to be this:

std::uint32_t DecCounter1() noexcept
{
    return m_Counter1.fetch_sub(1, std::memory_order_relaxed) - 1;
}