Segfault occuring in attempt to synchronize queue

Question

I am learning about multithreading and I wanted to simulate producer-consumer problem ( using semaphore if I can call it that ).

I have a class that holds a queue, producer push ints into queue and consumer retrieves it and prints it. I simulated is as following

class TestClass{
public:
    void producer( int i ){
        unique_lock<mutex> l(m);
        q.push(i);
        if( q.size() )
            cnd.notify_all();
    }

    void consumer(){
        unique_lock<mutex> l(m);
        while( q.empty() ){
            cnd.wait(l);
        }
        int tmp = q.front();
        q.pop();
        cout << "Producer got " << tmp << endl;
    }
    void ConsumerInit( int threads ){
        for( int i = 0; i < threads; i++ ){
            thrs[i] = thread(&TestClass::consumer, this );
        }
        for( auto &a : thrs )
            a.join();
    }


private:
    queue<int> q;
    vector<thread> thrs;
    mutex m;
    condition_variable cnd;
};

And I used a little console application to call data:

int main(){
    int x;   
    TestClass t;
    int counter = 0;
    while( cin >> x ){
        if( x == 0 )
            break;
        if( x == 1)
            t.producer(counter++);
        if( x == 2 )
            t.ConsumerInit(5);
    }   
}

So when user input 1, a data is pushed into the queue , if user press 2 threads are spawned.

In any order of invoking it, for example, pressing 1 1 and then 2, or 2 1 1 it throws segfault. I am not sure why my understanding of my code is as following: let's assume order 2 1 1

I initialize 5 threads, they see that queue is empty, so they go to sleep. When I push a number to the queue, it notifies all threads sleeping. The first one to wake up lock mutex again and proceed to retrieve number from queue and afterwards releasing the mutex, when mutex is released another thread do the same and unlocks the mutex, the third thread after mutex is unlocked is still in loop and see that queue is yet again empty and goes to sleep again, same with all remaining threads.

Is this logic correct? If so, why does this keep throwing segfault, if not, I appreciate all explanations.

Thanks for the help!

//edit By answers suggets , i replaced [] with vector.push_back , but consumer does nothing with data now , does not take it or print it.

score 1 · Accepted Answer · answered Mar 15 '17 at 21:08

1

You aren't expanding the thrs vector when you do

thrs[i] = thread(&CTest::consumer, this );

You should do

thrs.emplace_back(&CTest::consumer, this);

That's where the crash would be.

answered Mar 15 '17 at 21:08

Art Yerkes

1,274
9
9

noticed it and added push_back , but the consumer does nothing with data now , it does not take it and print it. – Darlyn Mar 15 '17 at 21:14
ConsumerInit will block indefinitely if there aren't 5 inputs waiting (and therefore the program can't do any more input). Are you inputting "2 1 1" ? – Art Yerkes Mar 15 '17 at 21:19
users inpu is 2 1 1 , so it creates threads , and then push number in queue twice – Darlyn Mar 15 '17 at 21:22
1

It creates 5 threads and then waits for them to terminate, then inputs another number. – Art Yerkes Mar 15 '17 at 21:26
I see your point , however when i remove join() and input 1 1 1 1 1 1 2 , the 5 numbers are printed , cuz thred takes them , but how could i make them to print all numbers , in this case alll 6 of them , and after joining them , termiante threads when queue is empty? – Darlyn Mar 15 '17 at 21:34
@trolkura Input the data one by one instead of a single line. That will slow down things enough for you to see that consumer() is just sitting there in a wait state and producer() is never called. As a matter of fact, the loop in `main()` never iterates to the top for any input after the first one (run in Visual Studio 2015). Looks like you are relying on "speed", and not safeness. – PaulMcKenzie Mar 15 '17 at 21:37

PaulMcKenzie · Answer 2 · 2017-03-15T21:11:38.503

1

Your issue has nothing to do with multithreading. You are accessing a std::vector out-of-bounds:

  for (int i = 0; i < threads; i++) {
        thrs[i] = thread(&CTest::consumer, this);

  //...
  vector<thread> thrs;

The thrs vector is empty, and you're trying to access as if it has entries.

To show the error, use:

        thrs.at(i) = thread(&CTest::consumer, this);

and you will be greeted with a std::out_of_range exception instead of a segmentation fault.

edited Mar 15 '17 at 21:11

answered Mar 15 '17 at 21:09

PaulMcKenzie

34,698
4
24
45

noticed it and added push_back , but the consumer does nothing with data now , it does not take it and print it. – Darlyn Mar 15 '17 at 21:10
Then that is another issue altogether. – PaulMcKenzie Mar 15 '17 at 21:34

score 1 · Answer 3 · answered Mar 15 '17 at 21:34

Your program deadlocks, if the input sequence is not in the form of 1 1 1 1 1 ... 2. That is if the number if 1s preceding 2 is less than five.

Here is the reason:

If the total elements in queue size are less than 5 and the main thread calls consumerInit, some of the five created consumer threads will block waiting for the queue to receive elements. Meanwhile, the main thread blocks on the join operation. Since the main thread will be waiting for consumer threads to finish while some of those threads are waiting for data to consume, there will be no progress. Hence deadlock.

score 1 · Answer 4 · answered Mar 15 '17 at 21:47

Problem is here:

  for( auto &a : thrs )
        a.join();

Main thread gets blocked here after you enter 2 waiting for the consumers to finish. So after this point you think that you are entering inputs, while there is no cin happening.

Remove these two lines and then you can enter 1 and producer/consumer will do their job.

Segfault occuring in attempt to synchronize queue

4 Answers4