Should destructors be threadsafe?

Question

I was going through a legacy code and found the following snippet:

MyClass::~MyClass()
{
   EnterCriticalSection(&cs);

//Access Data Members, **NO Global** members are being accessed here


  LeaveCriticalSection(&cs);
}

I am wondering will it help by any chance to guard the destructor ?

Consider a scenario :

1. Thread1 - About to execute any of the member function which uses critical section
2. Thread2-  About to execute destructor.

If the order of execution is 1=>2 then it might work. But what if the order is reversed ?

Is it a design issue ?

Answer 1

I think you have a more fundamental problem. It shouldn't be legal to destroy your object on one thread while another thread is still calling member functions. This in itself is wrong.

Even if you successfully guard your destructor with critical sections, what happens when the other thread starts executing the remainder of the function? It will be doing so on a deleted object which (depending on it's allocation location) will be garbage memory or simple an invalid object.

You need to alter your code to ensure the object is not destructed while still in use.

Answer 2

Not going to make a difference. If, as you say, the order of the calls is reversed then you're calling a member function on a destructed object and that's going to fail. Synchronization can't fix that logical error (for starters, the the member function call would be trying to acquire a lock object that's been destructed).

Answer 3

I second the comment from Neil ButterWorth. Absolutely, the Entities responsible for deleting and accessing the myclass, should have a check on this.

This synchronisation will start actually from the moment the object of type MyClass is created.

Answer 4

I have seen a case with ACE threads, where the thread is running on an ACE_Task_Base object and the object is destroyed from another thread. The destructor acquires a lock and notifies the contained thread, just before waiting on a condition. The thread that is running on the ACE_Task_Base signal signals the condition at exit and lets the destructor complete and the first thread exit:

class PeriodicThread : public ACE_Task_Base
{
public:
   PeriodicThread() : exit_( false ), mutex_()
   {
   }
   ~PeriodicThread()
   {
      mutex_.acquire();
      exit_ = true;
      mutex_.release();
      wait(); // wait for running thread to exit
   }
   int svc()
   {
      mutex_.acquire();
      while ( !exit_ ) { 
         mutex_.release();
         // perform periodic operation
         mutex_.acquire();
      }
      mutex_.release();
   }
private:
   bool exit_;
   ACE_Thread_Mutex mutex_;
};

In this code, the destructor must use thread safety techniques to guarantee that the object is not destroyed before the thread that is running svc() exits.

Answer 5

If you're accessing global variables you might need thread safety, yes

eg. My "Window" class adds itself to the list "knownWindows" in the constructor and removes itself in the destructor. "knownWindows" needs to be threadsafe so they both lock a mutex while they do it.

On the other hand, if your destructor only accesses members of the object being destroyed, you have a design issue.

Answer 6

Your comments says "NO Global members are being accessed here" so I'd guess not. Only the thread that created an object should destroy it, so from what other thread would you be protecting it?

I like orderly creation and destruction myself, where only a single object ever owns another sub-object, and any other object with a reference to that sub-object is a descendant further down in the tree. If any of those sub-objects represent different threads, then they will have made sure to complete before destruction proceeds up the tree.

Example:

• main() create object A
  • object A contains object B
    • object B contains object C
      • object C creates a thread that accesses objects A and B
      • object C's destructor runs, waiting for its thread to finish
    • object B's destructor runs
  • object A's destructor runs
• main() returns

The destructors for object A and B don't need to think about threads at all, and object C's destructor only needs to implement some communication mechanism (waiting on an event, for instance) with the thread it chose to create itself.

You can only get into trouble if you start handing out references (pointers) to your objects to arbitrary threads without keeping track of when those threads are created and destroyed, but if you're doing that then you should be using reference counting, and if you are then it's too late by the time the destructor is called. If there's still a reference to an object, then no one should have even tried to invoke its destructor.