Infinite loop scenario in custom throttler (race condition?)

Question

I wrote a custom throttler to handle scenarion when some client (that is making outside calls) is getting backoff exception and needs to sleep before making another API Call.

I have a memory dump with a strange scenario: stuck in an infinte loop, although I see no reason for it. This application has many threads, and all client calls are being throttled using the following code. Call to some API is done like this:

bool res = DoSomeAction([&]{ /* call client m_client.callAPI()*/ return true}

The memory dump shows only 1 thread is working (usually there are 10), and the m_isThrottling is set to true, so the entire application is running forever.

How can this scenario happen? Any suggestion for better implementation (variables with m_ prefix mean class variables, m_throttlingTime and m_isThrottling are static class variables)?

template<typename T>
bool ThrottlerClass::DoSomeAction(T && lambda)
{
    for (int retriesCount = 3; retriesCount > 0; --retriesCount)
    {
        while (m_isThrottling) //dump is stuck here
        {
            Sleep(10000);
        }

        try
        {
            return lambda();
        }
        catch (const std::exception& ex)
        {
            int time = m_client->GetThrottlingTimeMS(); //the client got exception making API call and saves it

            if (time > 0)
            {
                ExclusiveLock lock(&m_throttlingMutex); //custom scope mutex
                m_isThrottling = true;
                if (time > m_throttlingTime) 
                    m_throttlingTime = time;
            }

            if (m_throttlingTime > 0)
            {
                Sleep(m_throttlingTime);

                {
                    ExclusiveLock lock(&m_throttlingMutex);
                    m_isThrottling = false;
                    m_throttlingTime = 0;
                }
            }

            continue;
        }
    }

    return false;
}

Michael Veksler · Accepted Answer · 2020-05-17T13:36:27.827

1

There is no guarantee that the value of m_isTrhottling in your thread will ever get its true value. In fact, every thread has its own view of the world, so that in the view of the other thread, that writes to m_isThrottling, the value is true, but your third sees a different picture.

Locks is one way in which the world-view of the threads gets in sync. Had you used a lock, instead of the loop, the value will have been updated in your waiting thread.

Fix :

Either use a lock
Or std::atomic<bool> which also guarantees synchronization.

Clarification edit: The keyword static in the context of class variables, has no relevance for the memory model. In this case, the keyword simply states that the variable belongs to the class and not to any specific object. This is completely irrelevant to the synchronization of the world-view (or lack of) of threads. The only place where static is relevant to threading, is for static-variable initialization inside functions, and this is not the case (neither function static-variable, nor initialization)

edited May 17 '20 at 13:36

answered May 17 '20 at 11:50

Michael Veksler

8,217
1
20
33

m_isThrottling and m_throttlingTime are static variables, and I am using locks on writes i'm trying to minimize locks for performaces – damule May 17 '20 at 11:54
But you don't have a lock on read. The memory of the reading thread is not in sync with all other threads since there is no lock. You can use `std::atomic` to syncronize only one variable. As for it being static, it has no effect in this case. – Michael Veksler May 17 '20 at 12:10
But if m_isThrottling is static it should be the same for all threads. I would expect it to be set to false at some point (after throttling finished). How will a read lock help? It is stuck to true, with only 1 thread left forever. – damule May 17 '20 at 12:22
@damule I think that you are confusing C++ with another language. In C++ static has no impact in the memory model. Static only makes the variable belong to the class and not the object, such that there is only one copy shared among all objects of that class, and it can be accessed without having any object around. – Michael Veksler May 17 '20 at 12:54
@damule The only thing that links the keyword `static` with thread-safety is the initialization of static variables inside a function: https://stackoverflow.com/questions/1661529/is-meyers-implementation-of-the-singleton-pattern-thread-safe – Michael Veksler May 17 '20 at 12:57
I understand that the static is not thread safe and is shared across all of them But the writes are protected by a mutex, eventually while (m_isThrottling) should get the correct value. How could all the thread exit (except 1 thread stuck) without set m_isThrottling = true at some point? – damule May 17 '20 at 13:30
@damule there is no guarantee that the thread will ever read the updated data, unless there is a lock, or something else that guarantees synchronization. Having the mutex protect the write only guarantees synchronization with threads that also protect their write. If you don't want a mutex, try `std::atomic` – Michael Veksler May 17 '20 at 13:39
@damule if the compiler can peek into your Sleep() and see that there is no memory sync in there, it might keep m_isThrottling in a register (which will never be updated from the memory location of that variable). – Michael Veksler May 17 '20 at 13:42
So it is possible that the compiler will optimize while (m_isThrottling) and it will not update its value? That can explain the behavior. Will using volatile bool also help? – damule May 17 '20 at 13:55
@damule Even if the compiler is not smart enough to do this optimization, the hardware can still cause trouble. The hardware, can avoid syncing thread's memory, unless there is a synchronization instruction. – Michael Veksler May 17 '20 at 13:58

Infinite loop scenario in custom throttler (race condition?)

1 Answers1