Is it okay to use "delete this;" on an object that inherits from a Thread class?

Question

In general, if you have a class that inherits from a Thread class, and you want instances of that class to automatically deallocate after they are finished running, is it okay to delete this?

Specific Example:

In my application I have a Timer class with one static method called schedule. Users call it like so:

Timer::schedule((void*)obj, &callbackFunction, 15); // call callbackFunction(obj) in 15 seconds

The schedule method creates a Task object (which is similar in purpose to a Java TimerTask object). The Task class is private to the Timer class and inherits from the Thread class (which is implemented with pthreads). So the schedule method does this:

Task *task = new Task(obj, callback, seconds);
task->start(); // fork a thread, and call the task's run method

The Task constructor saves the arguments for use in the new thread. In the new thread, the task's run method is called, which looks like this:

void Timer::Task::run() {
    Thread::sleep(this->seconds);
    this->callback(this->obj);
    delete this;
}

Note that I can't make the task object a stack allocated object because the new thread needs it. Also, I've made the Task class private to the Timer class to prevent others from using it.

I am particularly worried because deleting the Task object means deleting the underlying Thread object. The only state in the Thread object is a pthread_t variable. Is there any way this could come back to bite me? Keep in mind that I do not use the pthread_t variable after the run method finishes.

I could bypass calling delete this by introducing some sort of state (either through an argument to the Thread::start method or something in the Thread constuctor) signifying that the method that is forked to should delete the object that it is calling the run method on. However, the code seems to work as is.

Any thoughts?

Answer 1

I think the 'delete this' is safe, as long as you don't do anything else afterwards in the run() method (because all of the Task's object's member variables, etc, will be freed memory at that point).

I do wonder about your design though... do you really want to be spawning a new thread every time someone schedules a timer callback? That seems rather inefficient to me. You might look into using a thread pool (or even just a single persistent timer thread, which is really just a thread pool of size one), at least as an optimization for later. (or better yet, implement the timer functionality without spawning extra threads at all... if you're using an event loop with a timeout feature (like select() or WaitForMultipleObjects()) it is possible to multiplex an arbitrary number of independent timer events inside a single thread's event loop)

Answer 2

There's nothing particularly horrible about delete this; as long as you assure that:

1. the object is always dynamically allocated, and
2. no member of the object is ever used after it's deleted.

The first of these is the difficult one. There are steps you can take (e.g. making the ctor private) that help, but nearly anything you do can be bypassed if somebody tries hard enough.

That said, you'd probably be better off with some sort of thread pool. It tends to be more efficient and scalable.

Edit: When I talked about being bypassed, I was thinking of code like this:

class HeapOnly {
  private:
    HeapOnly () {} // Private Constructor.
    ~HeapOnly () {} // A Private, non-virtual destructor.
  public:
    static HeapOnly * instance () { return new HeapOnly(); }
    void destroy () { delete this; }  // Reclaim memory.
};

That's about as good of protection as we can provide, but getting around it is trivial:

int main() { 
    char buffer[sizeof(HeapOnly)];

    HeapOnly *h = reinterpret_cast<HeapOnly *>(buffer);
    h->destroy(); // undefined behavior...
    return 0;
}

When it's direct like this, this situation's pretty obvious. When it's spread out over a larger system, with (for example) an object factory actually producing the objects, and code somewhere else entirely allocating the memory, etc., it can become much more difficult to track down.

I originally said "there's nothing particularly horrible about delete this;", and I stand by that -- I'm not going back on that and saying it shouldn't be used. I am trying to warn about the kind of problem that can arise with it if other code "Doesn't play well with others."

Answer 3

If all you ever do with a Task object is new it, start it, and then delete it, why would you need an object for it anyway? Why not simply implement a function which does what start does (minus object creation and deletion)?

Answer 4

delete this frees the memory you have explicitly allocated for the thread to use, but what about the resources allocated by the OS or pthreads library, such as the thread's call stack and kernel thread/process structure (if applicable)? If you never call pthread_join() or pthread_detach() and you never set the detachstate, I think you still have a memory leak.

It also depends on how your Thread class is designed to be used. If it calls pthread_join() in its destructor, that's a problem.

If you use pthread_detach() (which your Thread object might already be doing), and you're careful not to dereference this after deleting this, I think this approach should be workable, but others' suggestions to use a longer-lived thread (or thread pool) are well worth considering.