Crash when method of class is executing but smart pointer deleted the object

Question

I faced a problem with C++ memory management and smart pointers. I have a code to demonstrate you the problem:

#include <memory>

class Closeable
{
 public:
  virtual void Close() = 0;
};

class DisconnectionHandler
{
 public:
  virtual void HandleDisconnection() = 0;
};

class EventHandler
{
 public:
  virtual void HandleEvent() = 0;
};

class Notifier
{
 public:
  virtual void OnDisconnection() = 0;
};

class RemoteSystem : public Closeable
{
 public:
  void SetReceiveDataEventHandler(const std::shared_ptr<EventHandler>& receive_data_event_handler) {
    this->receive_data_event_handler_ = receive_data_event_handler;
  }

  void Close() override { this->receive_data_event_handler_ = nullptr; }

  // In this example to simplify the code I just call this method from the main function.
  void OnDataReceived() { this->receive_data_event_handler_->HandleEvent(); }

 private:
  std::shared_ptr<EventHandler> receive_data_event_handler_;
};

class ReceiveDataEventHandler : public EventHandler
{
 public:
  explicit ReceiveDataEventHandler(const std::shared_ptr<DisconnectionHandler>& disconnection_handler)
      : disconnection_handler_(disconnection_handler) {}

  void HandleEvent() override {
    // Some code of receiving data.
    // But we can find out that connection was closed and we must call the disconnection handler.
    if (this->IsConnectionClosed()) {
      this->disconnection_handler_->HandleDisconnection();
      return;
    }
    // Some other stuff..
  }

 private:
  [[nodiscard]] bool IsConnectionClosed() const {
    // In the example code I just return true.
    return true;
  }

 private:
  const std::shared_ptr<DisconnectionHandler> disconnection_handler_;
};

class RemoteSystemDisconnectionHandler : public DisconnectionHandler
{
 public:
  explicit RemoteSystemDisconnectionHandler(const std::shared_ptr<Closeable>& closeable_remote_system,
                                            Notifier* notifier)
      : closeable_remote_system_(closeable_remote_system), notifier_(notifier) {}

  ~RemoteSystemDisconnectionHandler() { printf("Destructed.\n"); }

  void HandleDisconnection() override {
    this->closeable_remote_system_->Close();
    printf("Closed.\n");
    this->notifier_->OnDisconnection();
    printf("Notified.\n");
  }

 private:
  const std::shared_ptr<Closeable> closeable_remote_system_;
  Notifier* const notifier_;
};

class ClientNotifier : public Notifier
{
 public:
  void OnDisconnection() override { printf("Disconnected.\n"); }
};

int main() {
  ClientNotifier notifier;

  auto remote_system = std::make_shared<RemoteSystem>();

  {
    // Scope for losing references in the main function after SetReceiveDataEventHandler.
    auto disconnection_handler = std::make_shared<RemoteSystemDisconnectionHandler>(remote_system, &notifier);
    auto receive_data_event_handler = std::make_shared<ReceiveDataEventHandler>(disconnection_handler);
    remote_system->SetReceiveDataEventHandler(receive_data_event_handler);
  }

  // Only in the example.
  remote_system->OnDataReceived();

  return 0;
}

You can also run this code. In this example program crashes on the line this->notifier_->OnDisconnection(). The output of the program:

Destructed.
Closed.
*crash*

This occurs because of losing the last reference to the ReceiveDataEventHandler when calling method RemoteSystem::Close from RemoteSystemDisconnectionHandler::HandleDisconnection, accordingly, losing the reference to the RemoteSystemDisconnectionHandler and deleting this object. After the Close method and deleting both objects of classes RemoteSystemDisconnectionHandler and ReceiveDataEventHandler it returns to the RemoteSystemDisconnectionHandler::HandleDisconnection method and prints 'Closed.' to the output, but since the object has been already deleted, the next line occurs an error, because now this is deleted and any access to it occurs memory exception. I also tried to rewrite this code on Java and it works fine, unlike C++. So, I want to ask you guys if there is a solution for this problem in the C++ community? I thought C++ had no problems with memory management since smart pointers exist, but appearently I was wrong. Hope for your help! Thanks in advance!

Answer 1

A simple solution is to make a copy of the shared_ptr before invoking the method on it:

    void OnDataReceived()
    { 
        auto temp = this->receive_data_event_handler_;
        if (temp)
        {
            temp->HandleEvent();
        }
    }

temp will keep the pointer alive until after the method invocation has completed.

However note that if you are using multiple threads in your real code, std::shared_ptr is not thread safe so you need to introduce a mutex to protect access to receive_data_event_handler_:

class RemoteSystem : public Closeable
{
public:
    void SetReceiveDataEventHandler(const std::shared_ptr<EventHandler>& receive_data_event_handler) {
        this->receive_data_event_handler_ = receive_data_event_handler;
    }

    void Close() override 
    { 
        std::unique_lock lock(mutex);
        this->receive_data_event_handler_ = nullptr;
    }

    // In this example to simplify the code I just call this method from the main function.
    void OnDataReceived()
    { 
        std::shared_ptr<EventHandler> temp;
        {
            std::unique_lock lock(mutex);
            temp = this->receive_data_event_handler_;
        }
        if (temp)
        {
            temp->HandleEvent();
        }
    }

private:
    std::shared_ptr<EventHandler> receive_data_event_handler_;
    std::mutex mutex;
};