Pure virtual function called C++ in base/derived class

Question

I have this base class:

class Base {
public:
  Base();
  virtual ~Base();
protected:
  virtual on_next_item(std::string& item) = 0;
private:
    void read_loop();
};

and this derived class:

class Derived : public Base {
public:
  Derived();
  virtual ~Derived();
protected:
  void on_next_item(std::string& item) override;
};

In the Base class constructor I'm starting a thread which reads from a socket and calls on_next_item() which is called on the derived class. In the Base destructor the reader thread is stopped via a atomic flag. But sometimes the read_loop still calls on_next_item and I get a "Pure virtual function called!" error. I assume I'm running in a race condition:

The subclass (object) has already been destructed and thus the function is not registered any more.

Is there a proper way to solve this race condition?

For completeness here's the reader loop:

while (running.load())
{
  string item = read();
  if (running.load())
  {
    on_text_item(item);
  }
}

The running flag is switched to false in the Base class destructor.

Edit (complete running example, has to be executed several times to run in the issue):

#include <atomic>
#include <boost/thread/thread.hpp>
#include <boost/chrono.hpp>
#include <iostream>

class Base
{
public:
  Base() : running(true)
  {
    readerThread = new boost::thread(&Base::read_loop, this);
  }

  virtual ~Base()
  {
    running = false;

    delete readerThread;
    readerThread = nullptr;
  }

protected:
  virtual void on_next_item(std::string &item) = 0;

private:
  boost::thread *readerThread;

  void read_loop()
  {
    std::string element = "Element";
    while (running.load())
    {
      boost::this_thread::sleep_for(boost::chrono::milliseconds(2));
      on_next_item(element);
    }
  }

  std::atomic_bool running;
};

class Derived : public Base
{
public:
  Derived() : Base()
  {
  }

  virtual ~Derived()
  {
  }

protected:
  virtual void on_next_item(std::string &item)
  {
    std::cout << "On Next Item " << item << std::endl;
  }
};

void constAndDestruct()
{
  Derived d;
  boost::this_thread::sleep_for(boost::chrono::seconds(2));
}

int main(int argc, char **argv)
{

  constAndDestruct();
  boost::this_thread::sleep_for(boost::chrono::seconds(2));
}

Thank you!

Can you provide a [MCVE] as usual please. Anything else would lead to pure speculation. — user0042, Aug 17 '17 at 07:41
You could move the logic from the destructor of `Base` to a member function instead. Or would that not work for you? — Maarten Bamelis, Aug 17 '17 at 07:43
Creating a thread in the constructor does not seem a good idea. Also in the Base class constructor, the object is not fully formed (derived does not exist yet). https://stackoverflow.com/questions/30258639/when-is-it-safe-to-call-this-in-constructor-and-destructor — mksteve, Aug 17 '17 at 07:46
I tried to follow RAII (https://en.wikipedia.org/wiki/Resource_acquisition_is_initialization). As a thread is a resource I wanted to init it in the constructor and destroy it in the destructor. Otherwise I would have to provide a "start" and a "stop" method. — Soccertrash, Aug 17 '17 at 07:52
It's natural that the [derived part is destructed before the base destructor gets called](https://stackoverflow.com/questions/7539282/order-of-calling-constructors-destructors-in-inheritance). Does the function need to be pure virtual? if you make the base implementation a no-op, then it is not an issue if it gets called after the derived part is destructed — slawekwin, Aug 17 '17 at 08:12
Note that you don't need to dynamically allocate the thread here. Just have it as a simple member — Caleth, Aug 17 '17 at 08:29
@slawekwin What do you mean by "no-op"? You mean providing a default implementation on the base class? — Soccertrash, Aug 17 '17 at 08:35
@Soccertrash yes, that's what I meant: `virtual void on_next_item(std::string &item) { }` — slawekwin, Aug 17 '17 at 09:35

score 6 · Answer 1 · answered Aug 17 '17 at 07:47

6

Calling virtual functions from constructors or destructors is generally considered a bad idea. The function call will actually be done as if the function were not virtual because at that point the constructor of Derived has not yet been called, member variables or Derived are still uninitialized...

The obvious solution is to move the logic of your class to a public member function and call that function just after creation of the object:

Derived d;
d.run();

answered Aug 17 '17 at 07:47

rodrigo

94,151
12
143
190

Basically I do not call a virtual function from the destructor. It is called from a method running on a thread. – Soccertrash Aug 17 '17 at 07:53
@rodrigo and the same goes for the object destruction: `Derived::~Derived()` should call a `Base::join()` method to be sure the thread calling `on_next_item()` is stopped before the `Derived` part of `d` is destroyed. – YSC Aug 17 '17 at 08:26
@Soccertrash You are calling a virtual function from a thead function started from a constructor. The virtual function may be called before or after the `Derived` constructor. It is a race, which is even worse. – rodrigo Aug 17 '17 at 08:57

score 0 · Answer 2 · answered Aug 17 '17 at 15:50

I would suggest the following changes-

Set the "running" flag to false inside ~Derived() rather than ~Base(). Change the access level of "running" to protected.

virtual ~Derived() { running = false; }
Remove the sleep from the reader's loop

while (running.load()) {
on_next_item(element); }

Pure virtual function called C++ in base/derived class

2 Answers2