3

I want want to implement a cmmand class which does some work in an another thread, and I don't want to let users to delete that object manually.My command class like this:

class Cmd {
 public:
  void excute() {
    std::cout << "thread begins" << std::endl;
    std::this_thread::sleep_for(std::chrono::seconds(2));  // do some work
    std::cout << "thread ends" << std::endl;
  }

  void run() {
    // I want std::unique_ptr to delete 'this' after work is done,but does't work
    std::thread td(&Cmd::excute, std::unique_ptr<Cmd>(this));
    td.detach();
  }

  // test if this object is still alive
  void ok() { std::cout << "OK" << std::endl; }
};

I use it like this:

int main() {
  Cmd *p = new Cmd();
  p->run();

  // waiting for cmd thread ends
  std::this_thread::sleep_for(std::chrono::seconds(3));

  p->ok();  // I thought p was deleted but not

  return 0;
}

As in the comments, the object is still alive after cmd thread finishes, I want to know how to implement such funtionality.

EDIT

users of cmd does't know when cmd will finish, so the flowing use case will leads to UB.

std::unique_ptr<Cmd> up(new Cmd);  // or just Cmd c;
up->run();
// cmd will be deleted after out of scope but cmd::excute may still need it

CLOSED

I made a mistake about test, in fact the object is deleted after the thread ends.It is more clear with following test with a additional member variable int i.

#include <functional>
#include <iostream>
#include <stack>
#include <thread>

using namespace std;

class Cmd {
 public:
  ~Cmd() { std::cout << "destructor" << std::endl; }

  void excute() {
    std::cout << i << " thread begins" << std::endl;
    std::this_thread::sleep_for(std::chrono::seconds(2));  // do some work
    std::cout << i << " thread ends" << std::endl;
  }

  void run() {
    // I want std::unique_ptr to delete 'this' after work is done,but it seems
    // not working
    std::thread td(&Cmd::excute, std::unique_ptr<Cmd>(this));
    td.detach();
  }

  // test if this object is still alive
  void ok() { std::cout << i << " OK" << std::endl; }

  int i;
};

int main() {
  Cmd *p = new Cmd();
  p->i = 10;
  p->run();

  // waiting for cmd thread ends
  std::this_thread::sleep_for(std::chrono::seconds(3));

  p->ok();  // I thought p was deleted but not

  return 0;
}

The flowwing outputs proved the object is deleted.

10 thread begins
10 thread ends
destructor
-572662307 OK

But just as some kind guys suggests, this is not a good design, avoid it as you can.

maidamai
  • 712
  • 9
  • 26
  • _"but doesn't work"_ ... Go on, give us the gory details. What happened? Would you be surprised if `Cmd *p = new Cmd(); delete p; p->ok()` printed your message? – Wyck Apr 24 '19 at 03:17
  • I would be surprised if `p->ok()` printed my message in your case,at least it is UB. – maidamai Apr 24 '19 at 03:52
  • I expect `thread` to delete `cmd` in my case. – maidamai Apr 24 '19 at 03:56
  • 1
    You're testing for UB. But if you have UB you can't rely on your test. Calling a member function on a deleted object is UB and not required to crash. `ok()` is falling at it's job. – François Andrieux Apr 24 '19 at 04:01
  • 1
    It's extremely unusual for an object to spontaneously steal it's own ownership and then end it's own lifetime. This type is easy to use incorrectly, making a design to avoid. One of the basic rules of good software design is to make code easy to use correctly and hard to use incorrectly. – François Andrieux Apr 24 '19 at 04:04
  • I assume you've tried it now that I've suggested it though. You see that it doesn't reliably detect whether or not the object is deleted, right? The OK message will still print when the object is deleted. I would advise against developing this _fire-and-forget_ pattern. Just use a smart pointer and join with the thread before cleaning up. It's not unreasonable for the caller to be notified when the command has completed. How much can you really do if you have a thread in flight? I doubt you could safely exit. And if you could, then maybe you want a process, not a thread. – Wyck Apr 24 '19 at 04:08
  • I just want to test if `cmd` is deleted, I will test this in destructor instead,thanks for your advice.@ François Andrieux – maidamai Apr 24 '19 at 04:13
  • @maidamai, Bad idea to "test if `cmd` is deleted". https://stackoverflow.com/questions/15730827/how-to-detect-if-a-pointer-was-deleted-and-securely-delete-it Better to just assign `nullptr` to `cmd` variable after calling `run`. Or better yet, make a smart command pointer class that follows that policy for you so you don't have to manage the raw Cmd* pointer. – Wyck Apr 24 '19 at 04:18

2 Answers2

1

Instead of thread, you can use std::future to signal the state. You can then either wait for the task to finish, or ignore the future completely.

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

class Cmd {
public:
    std::future<void> run() {
        std::lock_guard<std::mutex> lock(startMutex);
        if (started) {
            throw std::logic_error("already started");
        }
        started = true;

        // Take copies here, so that it doesn't matter if Cmd is destroyed
        int i_ = i;
        return std::async(std::launch::async, [i_]() {
            std::this_thread::sleep_for(std::chrono::seconds(2));
            std::cout << i_ << std::endl;
        });
    }

    int i = 0;

private:
    std::mutex startMutex;
    bool started = false;
};

int main() {
    auto p = std::make_unique<Cmd>();
    p->i = 10;
    auto f = p->run();
    p.reset();

    // Do some other work

    // Wait for the task to finish (or use f.get() if there is no need to
    // do some other work while waiting)
    if (f.valid()) {
        std::future_status operation;
        do {
            // Do some other work

            operation = f.wait_for(std::chrono::milliseconds(1));
        } while (operation != std::future_status::ready);
    }
}
VLL
  • 9,634
  • 1
  • 29
  • 54
0

There are two ways to automatically delete the memory created dynamically.

  1. In client code(main function), you should use some smart pointer like unique_pointer, so that once the object goes out of the scope, automatically it will be freed in unique_poiter destructor

  2. You can create your own smart pointer, it will be kind of wrapper around Cmd class. And has to overload a few operators. This smart pointer will also be taking care of dynamically allocated memory in the destructor. Client code should use this smart pointer while creating Cmd object dynamically.

santosh kumar
  • 585
  • 4
  • 14
  • the client code doen't know when the cmd finishes, and unique_ptror auto variables will delete the object premature which leads to undefined behaviour. – maidamai Apr 24 '19 at 03:18
  • 1
    @maidamai `std::move()` the `std::unique_ptr` into the `std::thread`, or use `std::shared_ptr` instead – Remy Lebeau Apr 24 '19 at 04:07
  • @maidamai What undefined behaviour do you get when the object gets deleted? It's totally fine for a member function of class to call `delete this` provided it doesn't touch its `this` pointer after doing so. It allows for sort of a "last one out turns the lights off" policy. You can delete the object and keep executing code in your method. Just don't call virtual methods or access member variables after deleting `this`. You should be fine. – Wyck Apr 24 '19 at 04:12
  • @Wyck the `cmd thread ` need to access some member variable(I don't demenstrate in the sample code), but the whole object has been deleted if you use auto variable of unique_ptr like `EDIT` part in my post – maidamai Apr 24 '19 at 05:20
  • @RemyLebeau Seems without `std::move()` the object is still deleted, I make a mistake about the test. – maidamai Apr 24 '19 at 05:40
  • @maidamai you are not passing the `unique_ptr` into `execute()` to keep it alive, so it dies immediately. You need to add a `unique_ptr` parameter to `execute()`, otherwise passing the `unique_ptr` to the `thread` constructor goes nowhere. `void excute(unique_ptr p) {...}; void run() { std::thread td(&Cmd::execute, this, std::unique_ptr(this)); td.detach(); }` – Remy Lebeau Apr 24 '19 at 16:10
  • 1
    @maidamai otherwise, do this instead: `void excute() { std::unique_ptr p(this); ...}; void run() { std::thread td(&Cmd::execute, this); td.detach(); }` – Remy Lebeau Apr 24 '19 at 16:12
  • @RemyLebeau from the test output at the last part of my post, the object does't die immediately, and I thought `unique_ptr` in `std::thread(&Cmd::excute, unique_ptr)` is an implicit parameter to `Cmd::excute`, am I right? – maidamai Apr 25 '19 at 01:05
  • @maidamai `execute()` is a *non-static* method, so the 2nd parameter of the `std::thread` constructor sets the `this` pointer for `execute()`, which needs to be a valid `Cmd*` pointer. That's why I passed the `std::unique_ptr` in the 3rd parameter of `std::thread`, to pass it as an explicit input parameter to `execute()`. I could be wrong, and what you did might work, I didn't test it. Hopefully `std:::thread` captures the `std::unique_ptr` and calls `execute()` on it correctly. I think you are moving into "undefined behavior" territory. My other suggested approaches should not invoke UB. – Remy Lebeau Apr 25 '19 at 02:45