views:

408

answers:

7

In c++, during a class constructor, I started a new thread with 'this' pointer as a parameter which will be used in the thread extensively (say, calling member functions). Is that a bad thing to do? Why and what are the consequences?

Edit: my thread start process is at the end of the constructor.

Thanks, Gil.

+7  A: 

The consequence is that the thread can start and code will start executing a not yet fully initialized object. Which is bad enough in itself.

If you are considering that 'well, it will be the last sentence in the constructor, it will be just about as constructed as it gets...' think again: you might derive from that class, and the derived object will not be constructed.

The compiler may want to play with your code around and decide that it will reorder instructions and it might actually pass the this pointer before executing any other part of the code... multithreading is tricky

David Rodríguez - dribeas
@gilbertc: If B derives from A, even if you launch the thread at the end of A's constructor, everything from B (vtable, member variables, constructor code) will not be initialized/executed. Could this be a nasty concurrency bug? At first glance, I would say yes, for the object will change in the construction thread, while it could be used in the newly created thread.
paercebal
+1  A: 

Depends on what you do after starting the thread. If you perform initialization work after the thread has started, then it could use data that is not properly initialized.

You can reduce the risks by using a factory method that first creates an object, then starts the thread.

But I think the greatest flaw in the design is that, for me at least, a constructor that does more than "construction" seems quite confusing.

Etienne de Martel
A: 

It's fine, as long as you can start using that pointer right away. If you require the rest of the constructor to complete initialization before the new thread can use the pointer, then you need to do some synchronization.

jeffamaphone
+1  A: 

Main consequence is that the thread might start running (and using your pointer) before the constructor has completed, so the object may not be in a defined/usable state. Likewise, depending how the thread is stopped it might continue running after the destructor has started and so the object again may not be in a usable state.

This is especially problematic if your class is a base class, since the derived class constructor won't even start running until after your constructor exits, and the derived class destructor will have completed before yours starts. Also, virtual function calls don't do what you might think before derived classes are constructed and after they're destructed: virtual calls "ignore" classes whose part of the object doesn't exist.

Example:

struct BaseThread {
    MyThread() {
        pthread_create(thread, attr, pthread_fn, static_cast<void*>(this));
    }
    virtual ~MyThread() {
        maybe stop thread somehow, reap it;
    }
    virtual void id() { std::cout << "base\n"; }
};

struct DerivedThread : BaseThread {
    virtual void id() { std::cout << "derived\n"; }
};

void* thread_fn(void* input) {
    (static_cast<BaseThread*>(input))->id();
    return 0;
}

Now if you create a DerivedThread, it's a best a race between the thread that constructs it and the new thread, to determine which version of id() gets called. It could be that something worse can happen, you'd need to look quite closely at your threading API and compiler.

The usual way to not have to worry about this is just to give your thread class a start() function, which the user calls after constructing it.

Steve Jessop
+1  A: 

It can be potentially dangerous.

During construction of a base class any calls to virtual functions will not despatch to overrides in more derived classes that haven't yet been completely constructed; once the construction of the more derived classes change this changes.

If the thread that you kick-off calls a virtual function and it is indeterminate where this happens in relation to the completion of the construction of the class then you are likely to get unpredictable behaviour; perhaps a crash.

Without virtual functions, if the thread only uses methods and data of the parts of the class that have been constructed completely the behaviour is likely to be predictable.

Charles Bailey
+1  A: 

I'd say that, as a general rule, you should avoid doing this. But you can certainly get away with it in many circumstances. I think there are basically two things that can go wrong:

  1. The new thread might try to access the object before the constructor finishes initializing it. You can work around this by making sure all initialization is complete before you start the thread. But what if someone inherits from your class? You have no control over what their constructor will do.
  2. What happens if your thread fails to start? There isn't really a clean way to handle errors in a constructor. You can throw an exception, but this is perilous since it means that your object's destructor will not get called. If you elect not to throw an exception, then you're stuck writing code in your various methods to check if things were initialized properly.

Generally speaking, if you have complex, error-prone initialization to perform, then it's best to do it in a method rather than the constructor.

Peter Ruderman
Not having the dtor called on construction failure is only perilous if your class directly handles raw resources - which it shouldn't do anyway. Since exceptions are the only way to report construction failures, they certainly shouldn't be banned in order to allow sloppily implemented classes to not to leak. I agree with everything else you say, though.
sbi
I didn't say it should be banned, only that it's perilous. It takes a degree of care to make sure everything is cleaned up properly in the face of an exception. This can be especially difficult when your class depends on code that you did not write and do not control.
Peter Ruderman
+1  A: 

Basically, what you need is two-phase construction: You want to start your thread only after the object is fully constructed. John Dibling answered a similar (not a duplicate) question yesterday exhaustively discussing two-phase construction. You might want to have a look at it.

Note, however, that this still leaves the problem that the thread might be started before a derived class' constructor is done. (Derived classes' constructors are called after those of their base classes.)

So in the end the safest thing is probably to manually start the thread:

class Thread { 
  public: 
    Thread();
    virtual ~Thread();
    void start();
    // ...
};

class MyThread : public Thread { 
  public:
    MyThread() : Thread() {}
    // ... 
};

void f()
{
  MyThread thrd;
  thrd.start();
  // ...
}
sbi