I am finding it very strange. Please, help me to explain this. I have a class which starts infinite loop in a separate thread, and two classes which inherit it. One of the classes implements the interface to be triggered outside as std::shared_ptr, and another one class hold this interface as std::weak_ptr. Please look at the code below. Sorry for a lot of code, I was trying to be as short as it possible to reproduce the error. Why sometimes have I pure virtual call in Sender::notify function? As far as I know std::shared_ptr is reentrant.
#include <iostream>
#include <memory>
#include <thread>
#include <atomic>
#include <list>
#include <mutex>
class Thread : private std::thread {
std::atomic_bool run {true};
public:
Thread() : std::thread([this](){ thread_fun(); }) {}
void thread_fun() {
while (run) loop_iteration();
}
virtual void loop_iteration() = 0;
virtual ~Thread() {
run.exchange(false);
join();
std::cout << "Thread released." << std::endl;
}
};
class Sender : public Thread {
public:
class Signal{
public:
virtual void send() = 0;
virtual ~Signal(){}
};
void add_receiver(std::weak_ptr<Signal> receiver) {
std::lock_guard<std::mutex> lock(receivers_mutex);
receivers.push_back(receiver);
}
void notify() {
std::lock_guard<std::mutex> lock(receivers_mutex);
for (auto r : receivers)
if (auto shp = r.lock())
shp->send(); //Somethimes I get the pure virtual call here
}
private:
std::mutex receivers_mutex;
std::list<std::weak_ptr<Signal>> receivers;
void loop_iteration() override {
std::this_thread::sleep_for(std::chrono::milliseconds(1000));
notify();
}
};
class Receiver : public Thread, public Sender::Signal {
std::atomic_bool notified {false};
public:
void send() override {
notified.exchange(true);
}
private:
void loop_iteration() override {
std::this_thread::sleep_for(std::chrono::milliseconds(250));
std::cout << "This thread was " << (notified? " " : "not ") << "notified" << std::endl;
}
};
int main() {
std::shared_ptr<Thread>
receiver = std::make_shared<Receiver>(),
notifier = std::make_shared<Sender>();
std::dynamic_pointer_cast<Sender>(notifier)->add_receiver(
std::dynamic_pointer_cast<Sender::Signal>(receiver));
receiver.reset();
notifier.reset();
return 0;
}
CodePudding user response：
Polymorphism doesn't work as you may expect during construction and destruction. The current type is the most derived type that still exists. When you are in Thread::~Thread the Sender part of your object has already been completely destroyed so it wouldn't be safe to call its overrides.
When thread_fun tries to run loop_iterator() before the constructor finishes or after the destructor starts, it will not polymorphically dispatch, but instead it will call Thread::loop_iteration which is a pure virtual function (= 0).
See https://en.cppreference.com/w/cpp/language/virtual#During_construction_and_destruction
Here is a demonstration of this : https://godbolt.org/z/4vsPGYq97
The derived object is destroyed after one second, at which point you see the output change indicating that the virtual function being called changes at that point.
I'm not sure if this code is valid, or if destroying the derived part of the object while one of its member function is being executed is Undefined Behavior.
CodePudding user response：
In addition to what François Andrieux noted, your real problem is that you are starting the thread running, using this object, before its construction is finished. It may or may not see the derived type constructed yet, depending on timing.
It's not calling thread_fun from the constructor, as he implies. It's calling that on a different thread, at some unknown point in the future. It might happen on a different core before this base class constructor has returned, or at any other random point during the derived class's construction process, or much later.
You can't safely start the thread's function until the object is ready to be used.
Separate creation from making it go. That's the easiest thing.
meanwhile
virtual ~Signal(){}
Don't define empty destructors. Write =default instead.
But, use override in the derived class, and don't use virtual there.
CodePudding user response：
You have a problem in that you assume that the spawned thread does not start immediately and the current thread has time to initialize the state of the current object before it does anything.
This does not hold which causes two issues.
- You accesses state in the current object that has not been initialized.
- You use a polymorphic function that is not guranteed to work until after the object is fully constructed.
You make a slight assumption in your destructor:
- You inherit from an object that does not have a virtual destructor.
- Your thread may still accesses state after the object has started its destruction. If it does (access destroyed) then it is UB. Your thread needs to be able to check if the current object state is valid (i.e. All derived classes must get a lock on
runand make sure its state is till true and all destructors must setrunto false.
Your problem lies here:
class Thread : private std::thread {
std::atomic_bool run {true};
public:
Thread()
// Here you are starting a separate thread of execution
// That calls the method thread_fun on the current object.
//
// No problem so far. BUT you should note that "this" object
// is not fully constructed at this point and there is no
// guarantees that the thread you just started will wait
// for this thread to finish before doing anything.
: std::thread([this](){ thread_fun(); })
{}
void thread_fun() {
// The new thread has just started to run.
// And is now accessing the variable `run`.
//
// But `run` is a member and initialized after
// the base class so you have no idea if the parent
// thread has correctly initialized this variable yet.
//
// There is no guratnee that the parent will get to
// the initialization point of `run` before this new thread
// gets to this point where it is using it.
while (run) {
// Here you are calling a virtual function.
// The trouble is that virtual functions are not
// guranteed to work as you would expect until all the
// constructors of the object have run.
// i.e. from base all the way to most derived.
//
// So you not only have to wait for this base class to
// be full constructed you must wait until the object
// is full constructed before you call this method.
loop_iteration();
}
}
virtual void loop_iteration() = 0;
virtual ~Thread() {
// You have a problem in that std::thread destructor
// is not virtual so you will not always call its destructor
// correctly.
//
// But lets assume it was called correctly.
// When you get to this point you have destroyed the
// the state of all derived parts of your object.
// So the function your thread is running better
// not touch any of that state as it is not all invalid
// and doing so is UB.
//
// If your object had no state then you are fine.
run.exchange(false);
join();
std::cout << "Thread released." << std::endl;
}
};
I think a better solution is to make the std::thread a member of your object, and force any threads to hold until you have the state correctly initialized (at the point where you create the object).
class Thread {
std::atomic_bool run;
std::thread thread;
public:
Thread(std::function<void>& hold)
// Make sure run is initialized before the thread.
: run{false}
, thread([this, &hold](){ thread_fun(hold); })
{}
void thread_fun(std::function<void>& hold) {
// Pass in a hold function.
// The creator of your objects defines this
// It is supposed to make objects created until you
// have all the state correctly set up.
// once it is you allow any threads that have called
// hold to be released so they can execute.
hold();
while (run) loop_iteration();
}
virtual void loop_iteration() = 0;
virtual ~Thread() {
run.exchange(false);
join();
std::cout << "Thread released." << std::endl;
}
};
Then you can create a simple barrier to use in hold:
class Barrier
{
bool threadsShouldWait = true;
std::conditional_variable cond;
std::mutex mutex;
void wait() {
std::unique_lock<std::mutex> lock(mutex);
cond.wait([&](){return !threadsShouldWait;}, lock);
}
void release() {
std::unique_lock<std::mutex> lock(mutex);
threadsShouldWait = false;
cond.notify_all();
}
}
int main()
{
// Note you don't need to use the same barrier for
// both these variables. I am just illustrating one use case.
Barrier barrier;
std::shared_ptr<Thread> receiver = std::make_shared<Receiver>([&barrier](){barrier.wait();});
std::shared_ptr<Thread> notifier = std::make_shared<Sender>([&barrier](){barrier.wait();});
barrier.release();