std::list threading push_back, front, pop_front

Question

Is std::list thread safe? I'm assuming its not so I added my own synchronization mechanisms (I think i have the right term). But I am still running into problems

Each function is called by a separate thread. Thread1 can not wait, it has to be as fast as possible

std::list<CFoo> g_buffer; 
bool g_buffer_lock; 

void thread1( CFoo frame ) {
    g_buffer_lock = true ; 
    g_buffer.push_back( frame ) ; 
    g_buffer_lock = false; 
}


void thread2( )
{
    while( g_buffer_lock ) {
     // Wait 
    }

    // CMSTP_Send_Frame * pMSTPFrame = NULL ; 
    while ( ! g_buffer_lock && g_buffer.size() > 0 )
    {
     // Get the top item 
     CFoo& pFoo = g_buffer.front() ;

     // Do something. 

     // remove the front item 
     g_buffer.pop_front();
    }
}

After about 170k calls to thread1 and 900k calls to thread2 I get an exception error on CFoo& pFoo = g_buffer.front() ;

That causes the program to crash. stdthrow.cpp: 22

#ifdef _DEBUG
_CRTIMP2_PURE void __CLRCALL_PURE_OR_CDECL _Debug_message(const wchar_t *message, const wchar_t *file, unsigned int line)
    { // report error and die
        if(::_CrtDbgReportW(_CRT_ASSERT, file, line, NULL, message)==1)
        {
            ::_CrtDbgBreak();
        }
    }
_CRTIMP2_PURE void __CLRCALL_PURE_OR_CDECL _Debug_message(const unsigned short *message, const unsigned short *file, unsigned int line)
    { // report error and die
        _Debug_message((wchar_t *) message, (wchar_t *) file, line);
    }

#endif

Suggestions, comments, is there a better way of doing things?

Answer 1

No, it's not guaranteed to be thread safe.

Your synchronization mechanism is flawed. You are allowing thread1 to change the list while thread2 is working with it. This can cause problems. Besides that, you should make your lock variable volatile.

Answer 2

Is std::list thread safe?

The current C++ standard doesn't even acknowledge the existence of threads, so std::list certainly isn't. Different implementations, however, might provide (different levels of) thread safety.

As for your code: If you need a lock, use a lock. That bool variable might not help when the threads are executed on different cores which fetch it from different caches. Use a real mutex instead.

Answer 3

You are correct in presuming that an stl list is not guaranteed to be thread-safe.

Also your synchronization mechanism isn't likely very good.

Inside thread2 you don't lock your bool, so you have a problem there.

You should likely as a minimum put a volatile qualifier in front of your lock bool; or better still look into real mutex functions appropriate to your platform.

Answer 4

Just because Thread1 needs to be as fast as possible, it doesn't mean that it is ok to let it access a list while it is being changed by another thread. Since both threads are modifying the list, both have to wait. Being fast doesn't help if you end up with corrupt data.

Edit:

Actually you may get away with it... Thread1 only adds elements to the list, while Thread2 only removes them. This means that Thread1 only needs to wait if the list only contains one element.

Edit2:

So, the way to make this work is for thread2 to lock the list if it only contains one element. It would have to check that before every deletion. This way thread1 would not have to wait except for that one case.

And you definitely should use a proper mutual exclusion mechanism (whatever is available on your platform) rather than a boolean flag.

Answer 5

If you really need thread1 to be as fast as possible, but still need thread safety, you can prevent some lock contention at the cost of a minimal amount of overhead, as such:

std::list<CFoo> g_buffer_thread1;
std::list<CFoo> g_buffer_thread2;
Mutex g_mutex; 

void thread1( CFoo frame ) {
    Locker lock( g_mutex );
    g_buffer_thread1.push_back( frame ) ; 
}


void thread2( )
{
    while( g_buffer_thread2.size() == 0 ) {
        // Wait?
        Locker lock( g_mutex );
        g_buffer_thread1.swap( g_buffer_thread2 );
    }

    while ( g_buffer_thread2.size() > 0 )
    {
        CFoo& pFoo = g_buffer_thread2.front() ;
        // Do something.
        g_buffer_thread2.pop_front();
    }
}

I think this is the most straightforward combination with thread safety. Thread1 must always lock, unfortunately. You might be able to come up with something where you batch frames for thread1. I'm assuming, based on your numbers in the question, that thread1 runs many more times than does thread2, so this will save some lock contention that would otherwise occur by using just one buffer list.