0

I have the following code:

#include <windows.h>
#include <iostream>

static DWORD __stdcall startThread(void *);

class BaseClass {
private:

    void threadLoop() {
        // stuff ...
        std::cout << someStuff() << std::endl;
        // stuff ...
    }
protected:
    HANDLE handle;
    virtual int someStuff() {
        return 1;
    }
public:
    friend DWORD __stdcall startThread(void *);

    BaseClass() {
        handle = 0;
    };

    void start() {
        handle = CreateThread(NULL, 0, startThread, this, 0, NULL);
    }

    ~BaseClass() {
        if(handle != 0) {
            WaitForSingleObject(handle, INFINITE);
            CloseHandle(handle);
        }
    }
    // stuff
};

static DWORD __stdcall startThread(void *obj_) {
    BaseClass *obj = static_cast<BaseClass *>(obj_);

    obj->threadLoop();
    return 0;
}

class DerivedClass : public BaseClass {
public:
    virtual int someStuff() {
        return 2;
    };
};

int main() {
    BaseClass base;
    base.start();
    DerivedClass derived;
    derived.start();
}

Every instance creates a thread using the WINAPI and the helper function startThread which delegates the call back to the method threadLoop of the object which created the thread. Now the problem is that threadLoop calls an other virtual method, but the polymorphism doesn't seem to work if I create a derived class with an other implementation of the virual method.

Why? How can I fix this?

Edit: I updated the code, so the thread doesn't get started in the constructor.

5
  • are you sure you are passing in an instance of the derived class to startThread()?
    – Nim
    Feb 18, 2013 at 11:55
  • 1
    you know that you can use portable, standardized C++11 threads now? std::async? No need to meddle with proprieary APIs or evil casts...
    – sehe
    Feb 18, 2013 at 11:55
  • @sehe And what makes you think he has access to C++11? Feb 18, 2013 at 11:56
  • @Nim how can I test this? Feb 18, 2013 at 11:59
  • @sehe I need to use the WINAPI Feb 18, 2013 at 12:00

2 Answers 2

6

You're starting the thread before you've finished constructing the derived object. This is undefined behavior (since you'll likely be accessing the object in the new thread while you're still executing code in the creating thread). You'll have to separate construction and starting the thread.

EDIT:

One way of handling this sort of problem:

class Threadable
{
public:
    virtual Threadable() {}
    virtual run() = 0;
};

DWORD __stdcall startThread( void* object )
{
    static_cast<Threadable*>( object )->run();
}

class Thread
{
    std::auto_ptr<Threadable> myThread;
    HANDLE myHandle;
public:
    Thread( std::auto_ptr<Threadable> thread )
        : myThread( thread )
        , myHandle( CreateThread( NULL, 0, startThread, myThread.get(), 0, NULL ) )
    {
    }
    ~Thread()
    {
        if ( myHandle != NULL ) {
            WaitForSingleObject( myHandle, INFINITE );
            CloseHandle( myHandle );
        }
    }
};

Then have your BaseClass and DerivedClass derive from Threadable, and invoke them as:

Thread base( std::auto_ptr<Threadable>( new BaseClass ) );
Thread derived( std::auto_ptr<Threadable>( new DerivedClass ) );

This isn't perfect (I don't like more or less unlimited waits in destructors), but it should be enough to get started. (Modulo any typos in the code above—I've not tested it.)

5
  • @Mogria Are you calling start from the constructor? If so, you still have the same problem. The point is that the construction of the derived class' sub-object doesn't start until your constructor has finished executing. That means that if you start your thread before the constructor has finished, your derived class may or may not be there when your thread "really" starts - i.e. it becomes a race condition whether it's there or not.
    – rlc
    Feb 18, 2013 at 12:08
  • @rlc no, I called start from the main function Feb 18, 2013 at 12:11
  • @ric Or expressed in the opposite way: you mustn't start the thread until the constructor of the most derived object has finished. There are several ways of achieving this: use the start method, and require client code to call it once the object has been completely constructed, don't use polymorphism in the thread object, but pass it a pointer to a fully constructed threadable, and some more complex solutions involving templates and multiple inheritance. Feb 18, 2013 at 12:11
  • @Mogria Could you update your question to show us exactly what you've done? Feb 18, 2013 at 12:12
  • @Mogria Second problem, you're not waiting to destruct until the thread has finished. You're waiting in the base class, which means that the derived class can be (and in a quick trial I just did, was) destructed before the thread ran. Same problem in reverse. My suggestion is to not make thread polymorphic, but to have it take a pointer to a polymorphic threadable. (I've edited my answer with some example code---off the top of my head, and untested, but you should get the idea.) Feb 18, 2013 at 14:19
1

There are several issues with your code, such as you're creating and running the thread while the object is being constructed. That is most definitely a bad design.

A clean design would be to encapsulate thread functionalities in an abstract class called thread, and then derive from it, overriding the run method, for example:

class thread : public noncopyable
{
protected:
    HANDLE m_hthread;
    unsigned long m_id;
private:
    static unsigned long __stdcall start(void* args)
    {
        static_cast<thread*>(args)->run();
        return 0;
    }
public:
    thread();
    virtual ~thread();
    virtual bool start()
    {
        if ( m_hthread != nullptr && isrunning(m_hthread) ) 
        {
           throw std::logic_error("Cannot start thread, as it is already running.");
        }
    m_hthread = ::CreateThread(NULL, 0, start, this, 0, &m_id);
    return m_hthread != nullptr;
    }
    unsigned long get_id() const;
    virtual unsigned long wait();
protected:
    virtual void run() = 0;
};

And derived from it:

class worker : public thread
{
  protected:
      virtual void run() override;
};

And you would use this as:

worker workerObject;
workerObject.start();

//do other works here
//maybe create few more threads;

workerObject.wait(); //wait for worker to complete!
4
  • I'm not sure that that's a very good design. Much better, I think, would be a separate Thread class, whose constructor takes a pointer to aThreadable` (or Runnable, or whatever) object. (But both work, as long as you don't start the thread in the constructor of the object which contains the code.) Feb 18, 2013 at 12:07
  • @JamesKanze: Agreed. It is better than the OP. But a much better design requires more thinking and typing, so I avoided it.
    – Nawaz
    Feb 18, 2013 at 12:08
  • @JamesKanze: The recommendation here is much better: drdobbs.com/parallel/prefer-futures-to-baked-in-async-apis/… . .NET design of Thread is class is much better than Java, though there are some issues with Resume functionality.
    – Nawaz
    Feb 18, 2013 at 12:17
  • It depends on why you're using a separate thread. Futures are very nice when you're parallizing the calculation of something, but they aren't very appropriate in some other cases (e.g. starting a connection thread in a server). Feb 18, 2013 at 12:20

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.