0

I have a class which has attribute thread. It looks something like this.

class myClass {
public:
      myClass(ClassB * x)  { 
           myThread = thread(&myClass::run, this); 
           classB = x;
      }
      ~myClass() { myThread.detach(); }
      void run() {
           while (something) {
              // do your work.
           }
           classB->endThisObject(this); 
      }
private:
    thread myThread;
    ClassB * classB;        
}

My classB looks like this.

ClassB {
public:
       endThisObject(myClass * x) { delete x; }
}

So basically the last operation of myThread is destroying itself using another object. Is it okey or this can cause many troubles ? I was testing in on my code and I had no leaks but this seems kinda wrong to me.

Druudik
  • 955
  • 1
  • 10
  • 32
  • Is there a specific reason you call `detach()` rather than `join()`? – Galik Mar 18 '18 at 11:37
  • If thread calls thread.join() on itself the deadlock will occur. – Druudik Mar 18 '18 at 11:40
  • 1
    But is the thread calling `join()`? All the thread needs to do is exit the loop. Then whatever destroys `myClass` will be calling `join()` through its destructor. – Galik Mar 18 '18 at 11:41
  • In fact the thread can't be the owner of `myClass` because the thread is created **by** `myClass`. So `myClass` destructor should be able to call `join()` on the thread I would think. – Galik Mar 18 '18 at 11:45
  • Ah I see what you mean. Well I would say that's not a good way to end your thread. You should simply change `something` to `false` to make the loop exit and then `join()` to wait for the thread to end. – Galik Mar 18 '18 at 11:49
  • I was trying to join that thread and I got deadlock. And that is because the thread which is trying to delete the object is waiting on itself when it finishes deleting object. – Druudik Mar 18 '18 at 11:50
  • I know that is not the best way but in my code this is the simplest thing to do. I was just wondering if this can cause any troubles/unexpected behaviour. – Druudik Mar 18 '18 at 11:51
  • I suppose the main concern is if the thread were to access member variables after the destructor started killing them. In this code it looks to me like you would be ok there. But it feels kind of *brittle*. – Galik Mar 18 '18 at 11:55

1 Answers1

1

Apart from threading, just look at this code:

B b;
MyClass myclass(&b);

The B instance will blindly and unmercifully call delete on a pointer to an object which had not been created with new.

Also, it should be noted that your run function is basically calling delete this, which is legal but (at least) a bit controversial (see here, for example).

I would give the MyClass class a start and a join method, like these:

   void MyClass::start()   
   {
       myThread = thread(&myClass::run, this);
   } 

   void MyClass::join()   
   {
       myThread.join();
   }

These way you can control the thread execution from outside the class and manage memory in simpler and safer ways:

   MyClass myclass;
   myclass.start();
   myclass.join();
   //no need to call delete, here

or

   auto p = std::make_unique<MyClass>();
   p->start();
   p->join();
   //done (and no delete, again)
p-a-o-l-o
  • 9,807
  • 2
  • 22
  • 35
  • In my code ClassB has array of myClass objects and only that one thread in myClass object can decide when is the time to delete "itself". Of course I can set some flags and let ClassB decide when is the time to actually delete "dead/flagged" myClass object and remove it from the array but this way it is much simplier and all I wanted to know is if it can cause some unexpected behaviour. – Druudik Mar 18 '18 at 13:03
  • @User5468622 Or, perhaps, you could use a condition variable that the threads can use to signal they are over and ready to be deleted? – Galik Mar 18 '18 at 14:09