C++ Question

[kinderchocolate]

Hi,

In Stockfish, I see

Thread::Thread() {
if (!thread_create(handle, start_routine, this))
{
......
}
}

start_routine makes a call to a pure virtual function.

So we're in the constructor and the start_routine might or might not be called before the constructor ends. We have a problem if it's called before the constructor returns because the derived version is not "ready".

Is this a bug?

[sje]

start_routine makes a call to a pure virtual function.

No, it's not a pure virtual method. This is virtual:

Code: Select all

virtual void idle_loop();

This is pure virtual:

Code: Select all

virtual void idle_loop() = 0;

[sje]

In Symbolic the constructor for a thread instance (or any derived classes) does NOT start (i.e., pthread_create) the thread, nor does the destructor finish (i.e, pthread_join) the thread.

Code: Select all

class Thread
{
public:
  Thread(void): isrunning(false) {}
  virtual ~Thread(void) {}

  virtual void RunLoop(void) = 0;

  void Start(void);
  void Finish(void);

  volatile bool isrunning;

private:
  static void *Starter(void *objptr);

  void *threadptr; // Pointer to the Posix thread object
};

And:

Code: Select all

void Thread::Start(void)
{
  threadptr = new pthread_t;
  pthread_attr_t *attrptr = new pthread_attr_t;

  pthread_attr_init(attrptr);
  pthread_attr_setdetachstate(attrptr, PTHREAD_CREATE_JOINABLE);
  if (pthread_create((pthread_t *) threadptr, attrptr, Thread::Starter, (void *) this))
    Die("Thread::Start", "pthread_create");
  pthread_attr_destroy(attrptr);
  delete attrptr;
}

void Thread::Finish(void)
{
  if (pthread_join(*(pthread_t *) threadptr, 0))
    Die("Thread::Finish", "pthread_join");
  delete (pthread_t *) threadptr;
}

void *Thread::Starter(void *objptr)
{
  ((Thread *) objptr)->isrunning = true;
  ((Thread *) objptr)->RunLoop();
  return 0;
}

And a sample:

Code: Select all

void Driver::RunIntCoPro(void)
{
  // class IntCoPro [Interactive Command Processor] inherits from CoPro
  // class CoPro [Command Processor] inherits from Task
  // class Task [General Task] inherits from Thread and EventList

  CoPro *coproptr = new IntCoPro();
  coproptr->Start();

  while (coproptr->isrunning)
    NapMsec(100);

  coproptr->Finish();
  delete coproptr;
  coproptr = 0;
}

The constructors for derived classes do lots more, but do not assume that the thread has actually started execution. The derived destructors also do not assume that the thread is running.

Note that because of the polymorphism, the destructors all need to be virtual.

I stole much of the above so long ago that I can't remember from where. But it all works.

[mar]

It is a very bad idea to start/join thread in class ctor/dtor IF you plan to derive a new class from that.
What can happen: thread physically starts before derived class finishes construction (very rarely but it can and ocasionally will) etc.
So it's only ok if the derived class does not need any extra construction/destruction, i.e. no extra objects (stl containers too) in member variables etc.
So I think you're right Ted (or Tim?:) and it's a bug.

[kinderchocolate]

It is a very bad idea to start/join thread in class ctor/dtor IF you plan to derive a new class from that.
What can happen: thread physically starts before derived class finishes construction (very rarely but it can and ocasionally will) etc.
So it's only ok if the derived class does not need any extra construction/destruction, i.e. no extra objects (stl containers too) in member variables etc.
So I think you're right Ted (or Tim?:) and it's a bug.

Martin, you're exactly right. I think it's a bug and is preventing the main engine thread to run. The main engine thread derives from the Thread class. The call to the virtual function can't be in the constructor. Moved to somewhere else, and now everything works.

[mar]

I will comment on why it's a bad idea to join in dtor:
in dtor (base thread), all member var objects of derived class are already destroyed but the thread may still be running!
Also in base class dtor the vtbl already points to base class methods, so calling virtual methods in base dtor is generally bad because it won't behave the way you'd normally expect (I think the same applies to ctor).

[mar]

The call to the virtual function can't be in the constructor.

It's actually not in the constructor, but in a static function that gets pointer to the class as parameter. In 99.99% cases the thread switch will occur after the construction completes so it will work in most cases. But we agree that it is a bug.
EDIT: why do I comment on that? Because I made the same mistake myself
EDIT2: the only correct way to encapsulate thread in a class is what Steven does

[kinderchocolate]

In Symbolic the constructor for a thread instance (or any derived classes) does NOT start (i.e., pthread_create) the thread, nor does the destructor finish (i.e, pthread_join) the thread.

Code: Select all

class Thread
{
public:
  Thread(void): isrunning(false) {}
  virtual ~Thread(void) {}

  virtual void RunLoop(void) = 0;

  void Start(void);
  void Finish(void);

  volatile bool isrunning;

private:
  static void *Starter(void *objptr);

  void *threadptr; // Pointer to the Posix thread object
};

And:

Code: Select all

void Thread::Start(void)
{
  threadptr = new pthread_t;
  pthread_attr_t *attrptr = new pthread_attr_t;

  pthread_attr_init(attrptr);
  pthread_attr_setdetachstate(attrptr, PTHREAD_CREATE_JOINABLE);
  if (pthread_create((pthread_t *) threadptr, attrptr, Thread::Starter, (void *) this))
    Die("Thread::Start", "pthread_create");
  pthread_attr_destroy(attrptr);
  delete attrptr;
}

void Thread::Finish(void)
{
  if (pthread_join(*(pthread_t *) threadptr, 0))
    Die("Thread::Finish", "pthread_join");
  delete (pthread_t *) threadptr;
}

void *Thread::Starter(void *objptr)
{
  ((Thread *) objptr)->isrunning = true;
  ((Thread *) objptr)->RunLoop();
  return 0;
}

And a sample:

Code: Select all

void Driver::RunIntCoPro(void)
{
  // class IntCoPro [Interactive Command Processor] inherits from CoPro
  // class CoPro [Command Processor] inherits from Task
  // class Task [General Task] inherits from Thread and EventList

  CoPro *coproptr = new IntCoPro();
  coproptr->Start();

  while (coproptr->isrunning)
    NapMsec(100);

  coproptr->Finish();
  delete coproptr;
  coproptr = 0;
}

The constructors for derived classes do lots more, but do not assume that the thread has actually started execution. The derived destructors also do not assume that the thread is running.

Note that because of the polymorphism, the destructors all need to be virtual.

I stole much of the above so long ago that I can't remember from where. But it all works.

Thanks. This fixes everything for me. : - )

[sje]

First, for stylistic correctness, a line should be added at the end of the Finish() routine:

Code: Select all

void Thread::Finish(void)
{
  if (pthread_join(*(pthread_t *) threadptr, 0))
    Die("Thread::Finish", "pthread_join");
  delete (pthread_t *) threadptr;
  threadptr = 0;  // To be consistent
}

Yet even in the unamended version, the Thread object can have multiple re-uses of sequential Start()/Finish() pairs of calls. This could be handy if the ctor/dtor does a lot of work that doesn't need repeated for sequential activations.

The isrunning instance variable is there for the outside world to see if the thread is still active. In the case of a Task instance, a true value means that the instance is still interested in processing incoming events on it's associated event list.

Generally, the isrunning variable is set to false by the Task instance when the task wants to say good-bye. For an instance of the interactive command processor task, the variable will be set false by an incoming EOF on the command input or by processing an exit command typed by the user. I'll probably add certain incoming signal kinds (i.e., SIG_HUP) as a third way of indicating a farewell request.

Over the years I've seen some really bad abuse of C++. I recall from long ago reading some C++ code which included the line:

Code: Select all

delete this;

Which to this day gives me the shivers considering how it could introduce all kinds of hard-to-debug randomness. Also, what if the instance was allocated on the stack or statically instead of on the heap? All because the guy who wrote it was too lazy to do it right in the first place.

[mcostalba]

Thanks. This fixes everything for me. : - )

Fixes what ?

You have not reported what your problem is.

Regrading starting the thread in c'tor, it is what std::thread does:

http://en.cppreference.com/w/cpp/thread/thread/thread


So we cannot assume it is the wrong thing just because we learnt to do it in another way.