2

Ran across this destructor in a codebase I am debugging.

ManagerImpl::~ManagerImpl() {
    // don't go away if some thread is still hitting us
    boost::unique_lock<boost::mutex> l(m_mutex);
}

Does it actually serve any useful purpose in a multi-threaded program? It looks like kludge.

I assume the idea is to defer destruction if another thread is calling a function that locks the mutex, but is it even effectively at doing that? ElectricFence segfaults would have me believe otherwise.

Cinder Biscuits
  • 4,880
  • 31
  • 51

1 Answers1

4

It is probably trying to postpone destruction until another thread unlocks the mutex and leaves another member function.

However, this wouldn't prevent another thread calling that function again after the lock in the destructor has been released.

There must be more interaction between threads (which you don't show) to make this code make sense. Still, thought, this doesn't seem to be robust code.

Maxim Egorushkin
  • 131,725
  • 17
  • 180
  • 271
  • 3
    Yep, looks like a quick patch for a design problem. Someone is accessing the object without properly owning or otherwise ensuring its lifetime. It might solve the problem for now, but like you say, if the interaction gets any more complex (like calling the function more than once) it will collapse again. – François Andrieux Jun 17 '19 at 16:58
  • Thanks for the confirmation. Indeed, the class is acting with global ownership, and this is one of several little hacks to patch a fundamentally poor design. – Cinder Biscuits Jun 17 '19 at 17:58