Search code examples
c++c++20

Why does my copy constructor/assignment does not work every time?

I have a class TimeStampQuery with copy and move constructors and assignments, I "tested positively" the result in unit tests, however I have a case where this does not work.

declarations:

typedef void(__stdcall* TimeStampCallback)(FF::TimestampPack tsp);
namespace FF
{
    struct __declspec(dllexport) TimestampPack
    {
        int TimeStamp;
        int ReaderIndex;

        TimestampPack(int ts, int index);
        std::wstring Towstring();
    };
}
class __declspec(dllexport) TimeStampQuery
{
protected:
    mutable std::mutex _mtx, mtxExit;
    std::condition_variable _cv, _cvExit;
    bool _run{ true };

    TimeStampCallback _tsCallback;
    FF::QueueStats* _lastStats{};

    std::queue<FF::TimestampPack> _queries;
    std::thread _threadCall;
public:

    TimeStampQuery(TimeStampCallback tscb);
    TimeStampQuery(const TimeStampQuery& other);
    TimeStampQuery(TimeStampQuery&& other) noexcept;
    TimeStampQuery& operator=(const TimeStampQuery& other);
    TimeStampQuery& operator=(TimeStampQuery&& other) noexcept;
    void swap(TimeStampQuery& other) noexcept;
    ~TimeStampQuery();
    void Add(int timestamp, int index);
    void Add(FF::TimestampPack tsp);

    void CallRunner();
    void WaitForFinish();
};

definitions:

FF::TimestampPack::TimestampPack(int ts, int index)
{
    TimeStamp = ts;
    ReaderIndex = index;
}

std::wstring FF::TimestampPack::Towstring()
{
    std::wstringstream wstr{};
    wstr << "timestamp " << TimeStamp << " from [" << ReaderIndex << "]";
    return wstr.str();
}
   
TimeStampQuery::TimeStampQuery(TimeStampCallback tscb)
    : _tsCallback(tscb), _threadCall([&] { CallRunner(); })
{
}

TimeStampQuery::TimeStampQuery(const TimeStampQuery& other):
    TimeStampQuery(other._tsCallback)
{
}

TimeStampQuery::TimeStampQuery(TimeStampQuery&& other) noexcept:
    _tsCallback(std::exchange(other._tsCallback, nullptr))
{
}

TimeStampQuery& TimeStampQuery::operator=(const TimeStampQuery& other)
{
    if (this == &other)
        return *this;
    TimeStampQuery(other).swap(*this);
    return *this;
}

TimeStampQuery& TimeStampQuery::operator=(TimeStampQuery&& other) noexcept
{
    TimeStampQuery(other).swap(*this);
    return *this;
}

void TimeStampQuery::swap(TimeStampQuery& other)  noexcept
{
    using std::swap;
    swap(_queries, other._queries);
}

TimeStampQuery::~TimeStampQuery()
{
    _run = false;
    _cv.notify_one();
    _threadCall.join();
}

void TimeStampQuery::Add(int timestamp, int index)
{
    FF::TimestampPack tsp(timestamp, index);
    Add(tsp);
}

void TimeStampQuery::Add(FF::TimestampPack tsp)
{
    std::lock_guard addLock(_mtx);
    _queries.push(tsp);
    _cv.notify_one();
}

void TimeStampQuery::CallRunner()
{
    while (true)
    {
        std::unique_lock readPopLock(_mtx);
        _cv.wait(readPopLock, [this] { return !_run || !_queries.empty(); });
        if (!_run)
            break;
        auto q = _queries.front();
        _queries.pop();
        _remaining--;
        readPopLock.unlock();
        _tsCallback(q);
        _done++;
        _cvExit.notify_one();
    }
}

void TimeStampQuery::WaitForFinish()
{
    while (true)
    {
        std::unique_lock exitLock(mtxExit);
        _cvExit.wait(exitLock, [this] { return _done == _total || _queries.empty(); });
        if (_done == _total)
            break;
    }
    _run = false;
    _cv.notify_one();
}

tests:

static MessageChangedCallback log = [](const wchar_t* z) { Logger::WriteMessage(z); };

static TimeStampCallback tscb = [](FF::TimestampPack tsp)
{
        Logger::WriteMessage(tsp.Towstring().c_str());
};

TEST_CLASS(TimeStampQueryTests)
{
public:
    TEST_METHOD(AddTest)
    {
        TimeStampQuery sut(tscb);
        sut.Add(33, 2);

        sut.WaitForFinish();
        Assert::AreEqual(1, sut.GetStats().Total);
        Assert::AreEqual(1, sut.GetStats().Done);
        Assert::AreEqual(0, sut.GetStats().Remaining);
    }

    TEST_METHOD(CopyTest)
    {
        TimeStampQuery sut(tscb);
        auto sut2 = sut;
        sut2.Add(34, 3);

        Assert::AreNotEqual((long long)&sut, (long long)&sut2);
        sut.WaitForFinish();
        sut2.WaitForFinish();
        Assert::AreEqual(1, sut2.GetStats().Total);
        Assert::AreEqual(1, sut2.GetStats().Done);
        Assert::AreEqual(0, sut2.GetStats().Remaining);
    }

    TEST_METHOD(MoveTest) // does not finish
    {
        TimeStampQuery sut(tscb);
        auto sut2 = std::move(sut);
        sut2.Add(34, 3);

        Assert::AreNotEqual((long long)&sut, (long long)&sut2);
        sut2.WaitForFinish();
        Assert::AreEqual(1, sut2.GetStats().Total);
        Assert::AreEqual(1, sut2.GetStats().Done);
        Assert::AreEqual(0, sut2.GetStats().Remaining);
    }
};

Case where copy assignment does not work:

#ifndef Pinvoke
#define Pinvoke extern "C" __declspec(dllexport)
#endif

Pinvoke auto __cdecl SetTimestampCallback(FFreader* ffreader, TimeStampCallback tscb) -> void
{
    TimeStampQuery tsq(tscb); // tsq._tsCallback == tscb // OK
    ffreader->TSquery = tsq; // ffreader->TSquery._tsCallback == 0x0000000000000000 // should not happen
} // break point

As commented above, I set a breakpoint a the end of the function, but it's as if the copy did not happen.

where TSquery was defined in FFreader like this:

class __declspec(dllexport) FFreader
{
    TimeStampQuery TSquery{nullptr};

If a proof is needed to convince:

tscb copy problem

Solution

  • your tests don't work in all conditions because your copy and move constructor and assignment operators are not actually copying or moving anything, the copy constructor needs to create a new thread and the move constructor needs to somehow redirect the thread to the new object, which is not done, but you have a fundamental bug.

    class __declspec(dllexport) TimeStampQuery
{
protected:
    mutable std::mutex _mtx, mtxExit;
    std::condition_variable _cv, _cvExit;
    bool _run{ true };
    int _done{};
    int _remaining{};
    int _total{};

    TimeStampCallback _tsCallback;
    FF::QueueStats* _lastStats{};

    std::queue<FF::TimestampPack> _queries;
    std::thread _threadCall;
public:

    TimeStampQuery(TimeStampCallback tscb);
    TimeStampQuery(const TimeStampQuery& other);
    TimeStampQuery(TimeStampQuery&& other) noexcept;
    TimeStampQuery& operator=(const TimeStampQuery& other);
    TimeStampQuery& operator=(TimeStampQuery&& other) noexcept;
    void swap(TimeStampQuery& other) noexcept;
    ~TimeStampQuery();
};

    your class has a mutex and a std::thread that is capturing this, your class is not relocatable, the thread depends on the object address being constant, it cannot be copied or moved, you shouldn't declare copy or move operations.

    TimeStampQuery(const TimeStampQuery& other) = delete;
TimeStampQuery(TimeStampQuery&& other) = delete;
TimeStampQuery& operator=(const TimeStampQuery& other) = delete;
TimeStampQuery& operator=(TimeStampQuery&& other) = delete;

    in fact you should omit them entirely and the compiler will implicitly delete them because of the mutex member which is not relocatable.

    the best you can do if you absolutely must have move operations is to put those non-relocatable parts into a heap allocated member with std::unqiue_ptr.

    struct TimeStampQueryState
{
    mutable std::mutex _mtx, mtxExit;
    std::condition_variable _cv, _cvExit;
    bool _run{ true };
    int _done{};
    int _remaining{};
    int _total{};

    TimeStampCallback _tsCallback;
    FF::QueueStats* _lastStats{};

    std::queue<FF::TimestampPack> _queries;
    std::thread _threadCall;
    ~TimeStampQueryState()
    {
      // stop the thread here
    };
};

class __declspec(dllexport) TimeStampQuery
{
    std::unique_ptr<TimeStampQueryState> m_state;
public:
    TimeStampQuery(TimeStampCallback tscb);
    ~TimeStampQuery() = default;
    TimeStampQuery(TimeStampQuery&&) = default;
    TimeStampQuery& operator=(TimeStampQuery&&) = default;
    // other methods here
};

    You can omit the move constructor and assignment operator and the compiler will implicitly generate them, always aim for the rule of zero.

    It is important that the thread now uses the address of TimeStampQueryState since that is not relocatable. the thread shouldn't capture this, in fact make the thread function a free function that takes TimeStampQueryState& as an argument to guarantee it cannot ever capture this by mistake.

    Don't Try to declare a copy constructor

    The existence of your class is associated with a thread, if your class is copied then a new thread should be created in the copy constructor. now the behavior of your program depends on whether copy elision is applied and the optimization level of your compiler, don't do that.

    The best you can do if you really need some copyability is implement a clone method, this way its caller knows that he is creating a thread and doing a lot of non-trivial work.

    if you need to pass it into a context that requires copyability such as std::function then wrap it with std::shared_ptr.

    You can replace the unique_ptr with shared_ptr and get copyability, but you are only getting reference copyability, not deep copyability, (the copied objects will be sharing the same state as the original object) it is up to you how to design your class.