Why is this c++ multithreading mutex code exhibiting occasional failures?

Question

I am using this foo.cpp code below on a linux Debian system:

#include <iostream>
#include <mutex>
#include <condition_variable>
#include <chrono>
#include <thread>

std::mutex mtx;
std::condition_variable cvar;
long next = 0;

void doit(long index){
  std::unique_lock<std::mutex> lock(mtx);
  cvar.wait(lock, [=]{return index == next;});

  std::cout<< index << std::endl;
  ++next;

  mtx.unlock();
  cvar.notify_all();

  return;
}

int main() 
{
  long n=50;

  for (long i=0; i < n; ++i)
    std::thread (doit,i).detach();

  while(next != n)
    std::this_thread::sleep_for(std::chrono::milliseconds(100));

  return(0);
}

I compile it with:

g++ -std=c++14 -pthread -o foo foo.cpp

It is designed to fire off 50 threads, detached, which are controlled by a mutex and condition_variable in the function doit, so they execute the mutex block sequentially.

It works most of the time, writing the numbers 00 to 49 to the screen, and then terminating.

However, it has two occasional failure modes:

Failure mode 1: After going up to some arbitrary number < 50, it aborts with the error:

foo: ../nptl/pthread_mutex_lock.c:80: __pthread_mutex_lock: Assertion `mutex->__data.__owner == 0' failed.

Failure mode 2: After going up to some arbitrary number < 50, it hangs, and has to be killed with ctrl-C to get back to the terminal prompt.

I would appreciate any suggestions as to the causes of this behavior, and how I can fix it.

=========================================================================

Edit: Ok, so here is a working revised version. I fixed the two bugs, and also changed the lock name from "lock" to "lk" to reduce confusion. Thanks for the help.

#include <iostream>
#include <mutex>
#include <condition_variable>
#include <thread>

std::mutex mtx;
std::condition_variable cvar;
long next = 0;

void doit(long index){

  std::unique_lock<std::mutex> lk(mtx);
  cvar.wait(lk, [=]{return index == next;});

  std::cout<< index << std::endl;
  ++next;

  lk.unlock();
  cvar.notify_all();

  return;
}

int main()
{
  long n=50;

  for (long i=0; i < n; ++i)
    std::thread (doit,i).detach();

  {
    std::unique_lock<std::mutex> lk(mtx);
    cvar.wait(lk, [=]{return n == next;});
  }

  return(0);
}

Answer 1

while(next != n) attempts to access variable next that can be modified by working threads without any sync creating a race condition. It should be covered by the same mutex:

{
   std::unique_lock<std::mutex> lock(mtx);
   cvar.wait(lock, [=]{return n == next;});
}

Detaching threads is not a good idea. You should store them somewhere and then join before returning from main.

Update: You are trying to call unlock on mutex itself instead of calling it on lock object. By constructing lock object you are delegating responsibility for unlocking mutex to lock object. It should be

lock.unlock();
cvar.notify_all();