is this code safe , is it ok to spawn a thread from a constructor C++?

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;
}

Solution

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.