C++11 thread to modify std::list

Question

I'll post my code, and then tell you what I think it's doing.

#include <thread>
#include <mutex>
#include <list>
#include <iostream>

using namespace std;

...

//List of threads and ints
list<thread> threads;
list<int> intList;

//Whether or not a thread is running
bool running(false);

//Counters
int busy(0), counter(0);

//Add 10000 elements to the list
for (int i = 0; i < 10000; ++i){
    //push back an int
    intList.push_back(i);
    counter++;
    //If the thread is running, make a note of it and continue
    if (running){
        busy++;
        continue;
    }
    //If we haven't yet added 10 elements before a reset, continue
    if (counter < 10)
        continue;
    //If we've added more than 10 ints, and there's no active thread,
    //reset the counter and launch
    counter = 0;
    threads.push_back(std::thread([&]
        //These iterators are function args
        (list<int>::iterator begin, list<int>::iterator end){
        //mutex for the running bool
        mutex m;
        m.lock();
        running = true;
        m.unlock();

        //Remove either 10 elements or every element till the end
        int removed(0);
        while (removed < 10 && begin != end){
            begin = intList.erase(begin);
            removed++;
        }

        //unlock the running bool
        m.lock();
        running = false;
        m.unlock();
        //Pass into the thread func the current beginning and end of the list
    }, intList.begin(), intList.end()));
}

for (auto& thread : threads){
    thread.join();
}

What I think this code is doing is adding 10000 elements to the end of a list. For every 10 we add, launch a (single) thread that deletes the first 10 elements of the list (at the time the thread was launched).

I don't expect this to remove every list element, I was just interested in seeing if I could add to the end of a list while removing elements from the beginning. In Visual Studio I get a "list iterators incompatible" error quite often, but I figure the problem is cross platform.

What's wrong with my thinking? I know it's something

EDIT:

So I see now that this code is very incorrect. Really I just want one auxiliary thread active at a time to delete elements, which is why I though calling erase was ok. However I don't know how to declare a thread without joining it up, and if I wait for that then I don't really see the point of doing any of this.

Should I declare my thread before the loop and have it wait for a signal from the main thread?

To clarify, my goal here is to do the following: I want to grab keyboard presses on one thread and store them in a list, and every so often log them to a file on a separate thread while removing the things I've logged. Since I don't want to spend a lot of time writing to the disk, I'd like to write in discrete chunks (of 10).

Thanks to Christophe, and everyone else. Here's my code now... I may be using lock_guard incorrectly.

#include <thread>
#include <mutex>
#include <list>
#include <iostream>
#include <atomic>

using namespace std;

...

atomic<bool> running(false);
list<int> intList;
int busy(0), counter(0);
mutex m;
thread * t(nullptr);

for (int i = 0; i < 100000; ++i){
    //Would a lock_guard here be inappropriate?
    m.lock();
    intList.push_back(i);
    m.unlock();
    counter++;
    if (running){
        busy++;
        continue;
    }
    if (counter < 10)
        continue;
    counter = 0;
    if (t){
        t->join();
        delete t;
    }
    t = new thread([&](){
        running = true;

        int removed(0);
        while (removed < 10){
            lock_guard<mutex> lock(m);
            if (intList.size())
                intList.erase(intList.begin());  
            removed++;
        }
        running = false;
    });
}

if (t){
    t->join();
    delete t;
}

Answer 1

Your code won't work for because:

• your mutex is local to each thread (each thread has it's own copy used only by itself: no chance of interthread synchronisation!)
• intList is not an atomic type, but you access to it from several threads causing race conditions and undefined behaviour.
• the begin and end that you send to your threads at their creation, might no longer be valid during the execution.

Here some improvements (look at the commented lines):

atomic<bool> running(false);    // <=== atomic (to avoid unnecessary use of mutex)
int busy(0), counter(0);
mutex l;   // define the mutex here, so that it will be the same for all threads

for (int i = 0; i < 10000; ++i){
    l.lock();    // <===you need to protect each access to the list
    intList.push_back(i);
    l.unlock();  // <===and unlock 
    counter++;
    if (running){
        busy++;
        continue;
    }
    if (counter < 10)
        continue;
    counter = 0;
    threads.push_back(std::thread([&]
        (){    //<====No iterator args as they might be outdated during executionof threads!!
        running = true;    // <=== no longer surrounded from lock/unlock as it is now atomic

        int removed(0);
        while (removed < 10){
            l.lock();       // <====you really need to protect access to the list
            if (intList.size())    // <=== check if elements exist NOW
                intList.erase(intList.begin());  // <===use current data, not a prehistoric outdated local begin !!
            l.unlock();      // <====end of protected section
            removed++;
        }

        running = false;    // <=== no longer surrounded from lock/unlock as it is now atomic
    })); //<===No other arguments
}
...

By the way, I'd suggest that you have a look at lock_guard<mutex> for the locks, as these ensure the unlock in all circumstances (especially when there are exceptions or orhter surprises like this).

Edit: I've avoided the lock protection of running with a mutex, by making it atomic<bool>.