0

I have a requirement to embed a thread inside a C++ class, kind of active object but not exactly. i am spawning thread from the constructor of the class , is it ok to do this way are there any problems with this approach. 

#include <iostream>
#include <thread>
#include <unistd.h>

class xfer
{
        int i;
        std::shared_ptr<std::thread> thr;
        struct runnable {
                friend class xfer;
                void operator()(xfer *x) {
                        std::cerr<<"thread started, xfer.i:"<<x->i;
                }
        } run;
        public:
        xfer() try : i(100), thr(new std::thread(std::bind(run, this))) { } catch(...) { }
        ~xfer() { thr->join(); }
};

int
main(int ac, char **av)
{
        xfer x1;
        return 0;
}
Marc Mutz - mmutz
  • 24,485
  • 12
  • 80
  • 90
Ravikumar Tulugu
  • 1,702
  • 2
  • 18
  • 40
  • `friend class xfer` is not doing what you think it does: it declares that `xfer` is a `friend` of `runnable` (which is useless) and not that `runnable` is a `friend` of `xfer` (which I guess was your intention). – Matthieu M. Jun 14 '12 at 11:43
  • the intent of declaring runnable as friend of xfer is to access the private state of xfer and kind of operator()() function to act like a method of xfer class. There is no way to pass member function to the thread. hence the declaration. – Ravikumar Tulugu Jun 14 '12 at 11:51
  • i have declared the way you advised and it also has the same effect. thanks – Ravikumar Tulugu Jun 14 '12 at 11:52
  • @RavikumarTulugu: `runnnable` is an inner class of `xfer`, so it already has access to the internals. No need for friendship there. – David Rodríguez - dribeas Jun 14 '12 at 12:06
  • I tried doing that but it seems this works only when the member variables of outer classes are static. – Ravikumar Tulugu Jun 14 '12 at 12:57
  • @Ravikumar : There's nothing magic about a nested type that makes it able to use instance members of the enclosing type without an actual _instance_ of the enclosing type... – ildjarn Jun 14 '12 at 17:25

3 Answers3

4

In general, there is no problem with starting a thread in a constructor, provided the objects used in the thread have been fully constructed before you start it. (Thus, for example, the idiom of having a thread base class, which starts the thread on itself in its constructor, is broken.) In your case, you haven't met this criteron, because the thread object uses your member run, which isn't constructed until after you've started the thread. Moving the creation of the thread into the body of the constructor, or simply changing the order of the data members in the class definition will correct this. (If you do the latter, do add a comment to the effect that the order is important, and why.)

Calling join in the destructor is more problematic. Any operation which may wait an indeterminate amount of time is, IMHO, problematic in a destructor. When a destructor is called during stack unwinding, you don't want to sit around waiting for the other thread to finish.

Also, you probably want to make the class uncopyable. (In which case, you don't need shared_ptr.) If you copy, you'll end up doing the join twice on the same thread.

James Kanze
  • 150,581
  • 18
  • 184
  • 329
  • 1
    "Calling `join` in the destructor is more problematic." Not doing so is even more problematic, since destroying a `std::thread` that is `joinable()` calls `std::terminate()` [thread.thread.destr]... – Marc Mutz - mmutz Jun 14 '12 at 12:35
  • @MarcMutz-mmutz Which may be preferable. In other cases, you may want to signal the thread that it should terminate, then `detach`. What you don't want to do is to have an indeterminate wait in a destructor. – James Kanze Jun 14 '12 at 12:45
  • how i plan to solve the problem is to stop the thread (by asking it) in the destructor before invoking join. this makes the join return in little time. however as you have mentioned it will delay the stack unwinding process a bit. – Ravikumar Tulugu Jun 14 '12 at 12:45
  • 1
    @RavikumarTulugu I'd also detach it. Even if you request its termination, it can't terminate until it has been scheduled, and you don't know when that will be. – James Kanze Jun 14 '12 at 13:37
  • @JamesKanze Then you'd leave the thread with a dangling pointer to xfer, which would invoke UB. How is that preferable? – Timothy003 Apr 20 '13 at 18:23
  • @Timothy003 The question is rather old, so I've forgotten the context. But: if the thread uses a pointer to a local variable, then you can't leave the local context without joining. That means usually that you must avoid operations which may throw between starting the thread and joining. It's a fair question, though, and it does require added thought. (In my own code, objects which are shared between threads are almost always allocated dynamically. But that could be an artifact of the types of applications I've worked on.) – James Kanze Apr 20 '13 at 20:16
2

Your code has a race condition and is not safe.

The problem is that initialization of the member variables happens in the order in which they are declared in the class, which means that the member thr is initialized before member run. During initialization of thr you are calling std::bind on run, and that will copy the (yet uninitialized) run object internally (did you know that you are copying there?).

Let's assume that you passed a pointer to run (or used std::ref to std::bind to avoid the copy), the code would still have a race condition. Passing the pointer/reference to std::bind would not be an issue in this case, as it is fine to pass a pointer/reference to a yet uninitialized member, as long as it is not accessed. But, there is still a race condition in that the std::thread object might spawn the thread too fast (say that the thread running the constructor gets evicted, and the new thread processes), and that can end up with the thread executing run.operator() before the run object has been initialized.

You need to reorder the fields in the type to make the code safe. And now that you know that a small change in the order of the members can have a disturbing effect on the validity of the code, you might also consider a safer design. (At this point it would be correct, and it might actually be what you need, but at least comment the class definition so that no one reorders the fields later)

David Rodríguez - dribeas
  • 204,818
  • 23
  • 294
  • 489
  • @RavikumarTulugu: Note the issue that James points out: destructors should not take an indeterminate time to complete (consider stack unwinding if an exception is thrown), and you need to make the type non-copyable (consider storing the `std::thread` directly in the class, rather than through a `shared_ptr`, else you are prone to subtle errors: a copy of the `xfer` object will lead to multiple `join()` calls on a single `thread` object which will probably bring your whole app down). Also consider accepting his answer. – David Rodríguez - dribeas Jun 14 '12 at 13:29
0

The use in the constructor might be safe but...

  • You should avoid using a bare new in a function call
  • The use of thr->join() in the destructor is... indicative of an issue

In details...

Do not use a bare new in function calls

Since the order of execution of the operands is unspecified, it is not exception-safe. Prefer using std::make_shared here, or in the case of unique_ptr, create your own make_unique facility (with perfect forwarding and variadic templates, it just works); those builder methods will ensure that the allocation and the attribution of the ownership to the RAII class are executed indivisibly.

Know the rule of three

The rule of three is a rule of thumb (established for C++03) which says that if you ever need to write a copy constructor, assignment operator or destructor, you should probably write the two others too.

In your case, the generated assignment operator is incorrect. That is, if I created two of your objects:

 int main() {
     xfer first;  // launches T1
     xfer second; // launches T2

     first = second;
 }

Then when performing the assignment I lose the reference to T1, but I never ever joined it!!!

I cannot recall whether the join call is mandatory or not, however your class is simply inconsistent. If it is not mandatory, drop the destructor; if it is mandatory, then you should be writing a lower-level class who only deals with one shared thread, and then ensure that the destruction of the shared thread is always preceeded by a join call, and finally use that shared thread facility directly in your xfer class.

As a rule of thumb, a class that has special copy/assign needs for a subset of its elements should probably be split up to isolate the elements with special needs in one (or several) dedicated classes.

Matthieu M.
  • 287,565
  • 48
  • 449
  • 722
  • *The use in the constructor might be safe but...* but it isn't. The order of initialization here breaks the correctness of the code. `std::bind` will copy from a yet uninitialized `run` member... Otherwise, this answer is a sound recommendation for the user (and others) to follow! (although I don't quite see the issue with the `shared_ptr` here) – David Rodríguez - dribeas Jun 14 '12 at 12:16
  • @DavidRodríguez-dribeas: exact, I cited the fact that the order of operations was unspecified but forgot to check if it affected anything else than the `new`. There is no issue with `new` *here*, because no other operation may throw, but in general if another of the operations executed in the initializer list could throw, then the call to `new` would be unsafe because the exception could occur between the end of `new` and the construction of the `shared_ptr`. – Matthieu M. Jun 14 '12 at 12:19
  • The order of operations in the initialization is well defined. Also, because the memory is managed directly in a `shared_ptr`, if `new` succeeds, then the `shared_ptr` acquires ownership of the memory and will release it if there is a later exception. Finally, there is no need for a pointer at all in the code, `std::thread` would very well be a member of the object and get initialized directly... unless the design requires that multiple copies of `xfer` share ownership of a single thread... – David Rodríguez - dribeas Jun 14 '12 at 12:22
  • To complete what @DavidRodríguez-dribeas is saying: there is a sequence point between each initialization, so all of the side effects of one initialization must be completed before the compiler can start on the next. Using `new` to initialize a smart pointer in an initialization list is always safe. – James Kanze Jun 14 '12 at 12:27
  • @DavidRodríguez-dribeas: I just remembered that `run` is stateless, I would guess it is still formally undefined, however in practice this looks fine, does not it ? – Matthieu M. Jun 14 '12 at 14:02
  • @JamesKanze: It is in this narrow case, but what if the `thr` construction was later expanded with a custom deleter whose creation could throw ? I see it as a trap for the unwary. – Matthieu M. Jun 14 '12 at 14:05
  • 1
    @MatthieuM. I was thinking about it when I wrote the above, but I did not want to make the comment more complicated than it is and assumed that it is an empty type only for illustration. Now, in reality and according to the standard, the code is actually well-behaved. For a type with a trivial constructor, the object lifetime starts as soon as the memory is allocated, so in this question, `run` is already alive *before* the thread is created, and the code is correct. That being said, I feared that stating that the code is correct would be read as a *blank* statement. – David Rodríguez - dribeas Jun 14 '12 at 14:18
  • @MatthieuM. That problem isn't particular to this use. You cannot safely invoke `std::shared_ptr( new T, D() )` if the constructor of `D` can throw. Period, and in no case. Any use of `shared_ptr` has this risk. (Hopefully, people who use explicit destructors are aware of this, and take precautions to avoid the problem.) – James Kanze Jun 14 '12 at 17:11