Home > other >  Destructor, when object's dynamic variable is locked by mutex will not free it?
Destructor, when object's dynamic variable is locked by mutex will not free it?

Time:08-09

I'm trying to solve some complicated (for me at least) asynchronous scenario at once, but I think it will be better to understand more simple case.

Consider an object, that has allocated memory, carrying by variable:

#include <thread>
#include <mutex>
using namespace std;

mutex mu;

class Object
{
public:
  char *var;

  Object()
  {
      var = new char[1]; var[0] = 1;
  }
  ~Object()
  {
      mu.lock();
      delete[]var; // destructor should free all dynamic memory on it's own, as I remember
      mu.unlock();
  }
}*object = nullptr;

int main()
{
   object = new Object();

   return 0;
}

What if while, it's var variable in detached, i.e. asynchronous thread, will be used, in another thread this object will be deleted?

void do_something()
{
    for(;;)
    {
         mu.lock();
         
         if(object)
             if(object->var[0] < 255)
                  object->var[0]  ;
             else
                  object->var[0] = 0;

         mu.unlock();
    }
}

int main()
{
   object = new Object();

   thread th(do_something);
   th.detach();

   Sleep(1000);

   delete object;
   object = nullptr;

   return 0;
}
  1. Is is it possible that var will not be deleted in destructor?
  2. Do I use mutex with detached threads correctly in code above?

2.1 Do I need cover by mutex::lock and mutex::unlock also delete object line?

I also once again separately point that I need new thread to be asynchronous. I do not need the main thread to be hanged, while new is running. I need two threads at once.

CodePudding user response:

Do I use mutex with detached threads correctly in code above?

Those are orthogonal concepts. I don't think mutex is used correctly since you only have one thread mutating and accessing the global variable, and you use the mutex to synchronize waits and exits. You should join the thread instead.

Also, detached threads are usually a code smell. There should be a way to wait all threads to finish before exiting the main function.

Do I need cover by mutex::lock and mutex::unlock also delete object line?

No since the destructor will call mu.lock() so you're fine here.

Is is it possible that var will not be deleted in destructor?

No, it will make you main thread to wait though. There are solutions to do this without using a mutex though.

There's usually two way to attack this problem. You can block the main thread until all other thread are done, or use shared ownership so both the main and the thread own the object variable, and only free when all owner are gone.

To block all thread until everyone is done then do cleanup, you can use std::barrier from C 20:

void do_something(std::barrier<std::function<void()>>& sync_point)
{
    for(;;)
    {

         
         if(object)
             if(object->var[0] < 255)
                  object->var[0]  ;
             else
                  object->var[0] = 0;


    } // break at a point so the thread exits
    sync_point.arrive_and_wait();
}

int main()
{
   object = new Object();

   auto const on_completion = []{ delete object; };

   // 2 is the number of threads. I'm counting the main thread since
   // you're using detached threads
   std::barrier<std::function<void()>> sync_point(2, on_completion);

   thread th(do_something, std::ref(sync_point));
   th.detach();

   Sleep(1000);
   sync_point.arrive_and_wait();

   return 0;
}

Live example

This will make all the threads (2 of them) wait until all thread gets to the sync point. Once that sync point is reached by all thread, it will run the on_completion function, which will delete the object once when no one needs it anymore.

The other solution would be to use a std::shared_ptr so anyone can own the pointer and free it only when no one is using it anymore. Note that you will need to remove the object global variable and replace it with a local variable to track the shared ownership:

void do_something(std::shared_ptr<Object> object)
{
    for(;;)
    {
         
         if(object)
             if(object->var[0] < 255)
                  object->var[0]  ;
             else
                  object->var[0] = 0;

    }
}

int main()
{
   std::shared_ptr<Object> object = std::make_shared<Object>();

   // You need to pass it as parameter otherwise it won't be safe
   thread th(do_something, object);
   th.detach();

   Sleep(1000);

   // If the thread is done, this line will call delete
   // If the thread is not done, the thread will call delete
   // when its local `object` variable goes out of scope.
   object = nullptr;

   return 0;
}

CodePudding user response:

Have a look at this, it shows the use of scoped_lock, std::async and managment of lifecycles through scopes (demo here : https://onlinegdb.com/FDw9fG9rS)

#include <future>
#include <mutex>
#include <chrono>
#include <iostream>

// using namespace std; <== dont do this
// mutex mu; avoid global variables.

class Object
{
public:
    Object() :
        m_var{ 1 }
    {
    }

    ~Object()
    {
    }

    void do_something()
    {
        using namespace std::chrono_literals;

        for(std::size_t n = 0; n < 30;   n)
        {
            // extra scope to reduce time of the lock
            {
                std::scoped_lock<std::mutex> lock{ m_mtx };
                m_var  ;
                std::cout << ".";
            }

            std::this_thread::sleep_for(150ms);
        }
    }


private:
    std::mutex m_mtx;
    char m_var;
    
};

int main()
{
    Object object;

    // extra scope to manage lifecycle of future
    {
        // use a lambda function to start the member function of object
        auto future = std::async(std::launch::async, [&] {object.do_something(); });

        std::cout << "do something started\n";
        
        // destructor of future will synchronize with end of thread;
    }
    std::cout << "\n work done\n";

    // safe to go out of scope now and destroy the object
    return 0;
}

CodePudding user response:

  1. Is is it possible that var will not be deleted in destructor?

With

~Object()
{
    mu.lock();
    delete[]var; // destructor should free all dynamic memory on it's own, as I remember
    mu.unlock();
}

You might have to wait that lock finish, but var would be deleted.

Except that your program exhibits undefined behaviour with non protected concurrent access to object. (delete object isn't protected, and you read it in your another thread), so everything can happen.

  1. Do I use mutex with detached threads correctly in code above?

Detached or not is irrelevant.

And your usage of mutex is wrong/incomplete. which variable should your mutex be protecting?

It seems to be a mix between object and var. If it is var, you might reduce scope in do_something (lock only in if-block)

And it misses currently some protection to object.

2.1 Do I need cover by mutex::lock and mutex::unlock also delete object line?

Yes object need protection.

But you cannot use that same mutex for that. std::mutex doesn't allow to lock twice in same thread (a protected delete[]var; inside a protected delete object) (std::recursive_mutex allows that).

So we come back to the question which variable does the mutex protect?

if only object (which is enough in your sample), it would be something like:

#include <thread>
#include <mutex>
using namespace std;

mutex mu;

class Object
{
public:
  char *var;

  Object()
  {
      var = new char[1]; var[0] = 1;
  }
  ~Object()
  {
      delete[]var; // destructor should free all dynamic memory on it's own, as I remember
  }
}*object = nullptr;

void do_something()
{
    for(;;)
    {
         mu.lock();
         
         if(object)
             if(object->var[0] < 255)
                  object->var[0]  ;
             else
                  object->var[0] = 0;

         mu.unlock();
    }
}

int main()
{
   object = new Object();

   thread th(do_something);
   th.detach();

   Sleep(1000);

   mu.lock(); // or const std::lock_guard<std::mutex> lock(mu); and get rid of unlock
   delete object;
   object = nullptr;
   mu.unlock();

   return 0;
}

Alternatively, as you don't have to share data between thread, you might do:

int main()
{
   Object object;

   thread th(do_something);

   Sleep(1000);

   th.join();
   return 0;
}

and get rid of all mutex

  • Related