This is a mock up of code I have for output drivers which output to files, database, etc. In the array in main, if I have two objects of the same child type, the code stalls out on the second call to ShutDown(). However, If I have two objects of different child types it does not stall out, exiting the program correctly. I have no idea what is causing the problem.

#include <iostream>
#include <fstream>
#include <list>
#include <thread>
#include <condition_variable>

using namespace std;

class Parent
{
public:
    Parent();
    virtual ~Parent() = default;
    virtual void ShutDown() = 0;

    void Push(int aInt);
    virtual void Write(int aInt) = 0;
    bool IsEmpty() { return mList.empty(); }
protected:
    std::list<int> Pop();
    void WriteWithThread();
    std::list<int> mList;
    std::thread mOutputThread;
    std::condition_variable mCV;
    std::mutex mMutex;
    std::atomic_bool mProgramRunning{ true };
    std::atomic_bool mThreadRunning{ false };

};

Parent::Parent()
{
    mOutputThread = std::move(std::thread(&Parent::WriteWithThread, this));
}

void Parent::Push(int aInt)
{
    std::unique_lock<std::mutex> lock(mMutex);
    mList.emplace_back(std::move(aInt));
    lock.unlock();
    mCV.notify_one();
}

std::list<int> Parent::Pop()
{
    std::unique_lock<std::mutex> lock(mMutex);
    mCV.wait(lock, [&] {return !mList.empty(); });
    std::list<int> removed;
    removed.splice(removed.begin(), mList, mList.begin());
    return removed;
}

void Parent::WriteWithThread()
{
    mThreadRunning = true;
    while (mProgramRunning || !mList.empty())
    {
        Write(Pop().front());
    }
    mThreadRunning = false;
}

class ChildCout : public Parent
{
public:
    ChildCout() = default;
    void ShutDown() override;
    void Write(int aInt) override;
};

void ChildCout::ShutDown()
{
    mProgramRunning = false;
    if (mOutputThread.joinable())
    {
        mOutputThread.join();
    }
    std::cout << "Shutdown Complete"<< std::endl;
}
void ChildCout::Write(int aInt)
{
    std::cout << "Inserting number: " << aInt << std::endl;
}
class ChildFile : public Parent
{
public:
    ChildFile(std::string aFile);
    void ShutDown() override;
    void Write(int aInt) override;
private:
    std::fstream mFS;
};

ChildFile::ChildFile(std::string aFile):Parent()
{
    mFS.open(aFile);
}

void ChildFile::ShutDown()
{
    mProgramRunning = false;
    if (mOutputThread.joinable())
    {
        mOutputThread.join();
    }
    mFS.close();
    std::cout << "Shutdown Complete" << std::endl;
}

void ChildFile::Write(int aInt)
{
    mFS<< "Inserting number: " << aInt << std::endl;
}

int main()
{
    Parent *array[] = {new ChildFile("DriverOutput.txt"),new ChildFile("Output2.txt"), new ChildCout()};

    for (int i = 0; i < 1000; i  )
    {
        for (auto& child : array)
        {
            child->Push(i);
        }
    }
    for (auto& child : array)
    {
        child->ShutDown();
    }

   return 0;
}

CodePudding user response：

You never notify the thread about the shutdown and the predicate doesn't even consider, whether mProgramRunning has been set to false. You need to notify the condition variable after writing mProgramRunning you need to change the predicate accordingly.

E.g. something like this should work, but I consider the separation of the Pop and the WriteWithThread functions suboptimal, since the signatures force you to add unnecessary reads to mProgramRunning.

...

std::list<int> Parent::Pop()
{
    std::unique_lock<std::mutex> lock(mMutex);
    //mCV.wait(lock, [&] {return !mList.empty(); });
    bool running = true;
    mCV.wait(lock, [this, &running] { // member variables accessed via this pointer -> capture this by value
        running = running = mProgramRunning.load(std::memory_order_acquire);
        return !running || !mList.empty(); // need to check for shutdown here too
    });

    std::list<int> removed;
    if (running)
    {
        removed.splice(removed.begin(), mList, mList.begin());
    }
    return removed;
}

void Parent::WriteWithThread()
{
    mThreadRunning = true;
    while (mProgramRunning)
    {
        auto list = Pop();
        if (list.empty())
        {
            break;
        }
        Write(list.front());
    }
    mThreadRunning = false;
}

...

void ChildCout::ShutDown()
{
    mProgramRunning.store(false, std::memory_order_release);
    mCV.notify_all(); // tell consumer about termination
    if (mOutputThread.joinable())
    {
        mOutputThread.join();
    }
    std::cout << "Shutdown Complete" << std::endl;
}

...

void ChildFile::ShutDown()
{
    mProgramRunning.store(false, std::memory_order_release);
    mCV.notify_all(); // tell consumer about termination
    if (mOutputThread.joinable())
    {
        mOutputThread.join();
    }
    mFS.close();
    std::cout << "Shutdown Complete" << std::endl;
}

...

Note: I'm not 100% sure this does what exactly you expect it to do, but it should give you an idea what went wrong.

A note on std::move: This is only useful to turn something not assignable to an rvalue reference into something that is assignable to an rvalue reference.

In

mOutputThread = std::move(std::thread(&Parent::WriteWithThread, this));

it's unnecessary to use std::move, since std::thread(&Parent::WriteWithThread, this) is an xvalue which is assignable to an rvalue reference, so

mOutputThread = std::thread(&Parent::WriteWithThread, this);

is sufficient.

You'd need use std::move, if the thread object "had a name":

std::thread tempThread(&Parent::WriteWithThread, this);
mOutputThread = std::move(tempThread);

Furthermore in

mList.emplace_back(std::move(aInt));

it's not necessary to use std::move either, copy and move assignment for int have the same effect. Arithmetic types don't benefit from the use of std::move.