Destroying condition_variable while waiting

You can delete it right away.

Quote from C++ standard:

~ condition_variable();

Requires: There shall be no thread blocked on *this. [Note: That is, all threads shall have been notified; they may subsequently block on the lock specified in the wait. This relaxes the usual rules, which would have required all wait calls to happen before destruction. Only the notification to unblock the wait must happen before destruction.

Basically wait functions are required to perform locking and waiting atomically:

The execution of notify_one and notify_all shall be atomic. The execution of wait, wait_for, and wait_until shall be performed in three atomic parts:

1. the release of the mutex and entry into the waiting state;
2. the unblocking of the wait; and
3. the reacquisition of the lock.

Once notify wakes a thread, it should be considered "unblocked" and should contest the mutex.

There are similar guarantees about std::mutex: threads are not required to leave unlock before mutex is destroyed.

Quote from C++ standard:

The implementation shall provide lock and unlock operations, as described below. For purposes of determining the existence of a data race, these behave as atomic operations. The lock and unlock operations on a single mutex shall appear to occur in a single total order.

Later:

Note: After a thread A has called unlock(), releasing a mutex, it is possible for another thread B to lock the same mutex, observe that it is no longer in use, unlock it, and destroy it, before thread A appears to have returned from its unlock call.

Such guarantees are required to avoid issues like this, when mutex inside an object is used to protect object reference counter.

Note that this does not guarantee that your implementation has no bugs in this regard. In the past glibc had multiple bugs related to the destruction of synchronization objects, in particular pthread_mutex_unlock was accessing mutex before returning.

One easy fix: move delete foo into thread 1 after foo->cv.wait(...);.

A better fix would be to change the design to work with std::shared_ptr, no manual delete invocations.

Here is my solution for C++17 (C++11 needs adaptation): notify everybody that we are deleting current instance and make the destructor to wait

Two things to do:

1. Give to the wait predicate a possibility to exit before object is really deleted
2. Make the destructor to resync with the lock to make it wait everything is finished (you must be sure all waiting methods are checking the deleting at beginning)

Note: if you have a thread inside this class waiting on the condition_variable, it might be better to join() the thread after notification instead of using the resynchronization lock (see comment in the code)

#include <iostream>
#include <memory>
#include <string>
#include <thread>
#include <condition_variable>
#include <mutex>
#include <shared_mutex>

using namespace std;

chrono::system_clock::time_point startTime;

struct Foo
{
    condition_variable cond;
    mutex mutex1;
    shared_mutex mutexSafe;

    bool deleting = false;

    ~Foo()
    {
        deleting = true;
        cond.notify_all();
        
        // Will make the destructor to wait all the opened shared_lock are released
        unique_lock l(mutexSafe);
    }

    void waitOnThing(const string& name)
    {
        // Shared lock to make possible several threads are using the method at the same time
        shared_lock lSafe(mutexSafe);

        cout << chrono::duration_cast<chrono::milliseconds>(chrono::system_clock::now() - startTime).count()
            << " Thread " << name << " -> waitOnThing()" << endl;

        unique_lock l(mutex1);
        cond.wait(l, [&]()
            {
                if (deleting)
                {
                    this_thread::sleep_for(chrono::milliseconds(1000)); // Slow down exit process to show destructor is waiting
                    return true;
                }
                return false;
            });

        cout << chrono::duration_cast<chrono::milliseconds>(chrono::system_clock::now() - startTime).count()
            << " Thread " << name << " unlocked" << endl;
    }
};



int main()
{
    startTime = chrono::system_clock::now();
    Foo* foo = new Foo();
    
    cout << chrono::duration_cast<chrono::milliseconds>(chrono::system_clock::now() - startTime).count()
        << " Starting" << endl;

    thread t1([&]() { foo->waitOnThing("t1"); });
    thread t2([&]() { foo->waitOnThing("t2"); });
    thread t3([&]() { foo->waitOnThing("t3"); });

    // Wait a bit to be sure thread started and is waiting
    this_thread::sleep_for(chrono::milliseconds(100));
    
    cout << chrono::duration_cast<chrono::milliseconds>(chrono::system_clock::now() - startTime).count()
        << " Deleting foo..." << endl;
    delete foo;

    cout << chrono::duration_cast<chrono::milliseconds>(chrono::system_clock::now() - startTime).count()
        << " Foo deleted" << endl;

    // Avoid demo to crash
    t1.join();
    t2.join();
    t3.join();
}

Result:

0 Starting
4 Thread t2 -> waitOnThing()
4 Thread t1 -> waitOnThing()
4 Thread t3 -> waitOnThing()
100 Deleting foo...
1100 Thread t1 unlocked
2100 Thread t2 unlocked
3100 Thread t3 unlocked
3100 Foo deleted

C++11 :

Class shared_mutex is available only since C++17, if you are using C++11 you can do the same by using a regular unique_lock and making a vector of mutex (one instance per call to the waiting method) and try to lock all of them in the destructor.

Example (not tested):

vector<shared_ptr<mutex>> mutexesSafe;

~Foo()
{
    // ...
    
    for(const auto& m : mutexesSafe)
    {
        unique_lock l(m);
    }
}
void waitOnThing(const string& name)
{
    auto m = make_shared<mutex>();
    mutexesSafe.push_back(m);
    unique_lock lSafe(*m);
    
    //...
}