Breaking a circular dependency between a shared_ptr and a unique_ptr

Question

Given this code:

#include <iostream>
#include <memory>

class Controller;

class View {
public:
    ~View() {
        std::cout << "Disposing View" << std::endl;
    }

    void SetObserver(std::shared_ptr<Controller> a_observer) {
        observer = a_observer;
    }
private:
    std::shared_ptr<Controller> observer;
};

class Controller : public std::enable_shared_from_this<Controller> {
public:
    static std::shared_ptr<Controller> Create(std::unique_ptr<View> view) {
        //Can't use std::make_shared due to visibility rules :(
        auto controller = std::shared_ptr<Controller>(new Controller(std::move(view)));

        controller->Init();

        return controller;
    }

    ~Controller() {
        std::cout << "Disposing Controller" << std::endl;
    }
private:
    std::unique_ptr<View> view;

    explicit Controller(std::unique_ptr<View> a_view) : view(std::move(a_view)) {}

    Controller(const Controller&) = delete;

    void Init() {
        view->SetObserver(shared_from_this());
    }
};

int main() {
    auto view = std::make_unique<View>();

    auto controller = Controller::Create(std::move(view));

    return 0;
}

I think that the controller object will never be disposed (confirmed by running it).

In order to alleviate this issue, is it sufficient to make the observer variable a weak_ptrinstead of a shared_ptr?

Besides this, is there any other potential issue that I should be aware of, given the above design?

*"Can't use std::make_shared due to visibility rules"* - See http://stackoverflow.com/questions/8147027/how-do-i-call-stdmake-shared-on-a-class-with-only-protected-or-private-const — Christian Hackl, Mar 12 '16 at 10:33
@ChristianHackl Thanks for linking that answer, but I think I'll stick to using `new` for now. I don't want to add too much clutter into the code. — Mihai Todor, Mar 13 '16 at 05:54

score 3 · Accepted Answer · edited May 23 '17 at 11:45

Yes, as you state about std::weak_ptr:

In addition, std::weak_ptr is used to break circular references of std::shared_ptr.

Changing the member to std::weak_ptr and running, yields

$ ./a.out 
Disposing Controller
Disposing View

When you need it, just call lock (checking the return value), to obtain a std::shared_ptr (which you should not store as a member):

void doSomethingWithView() {
    auto obs = observer.lock();                                                                                                                                                                          
    if(obs) {
        // Still valid
    }   
}

A possible caveat has to do with std::weak_ptr and multithreading.

Joseph Thomson · Answer 2 · 2016-03-12T11:41:45.113

Using a std::weak_ptr would be okay, but given that the lifetime of the View is tied to its Controller, you can just store a regular pointer to the Controller. This way, the Controller doesn't have to be stored in a std::shared_ptr, and you can get rid of the std::enable_shared_from_this two-stage initialization mess.

I would also make SetObserver private and make Controller a friend of View for safety reasons. They're already tightly coupled after all.

#include <memory>

class Controller;

class View {
  friend class Controller;

private:
    void SetObserver(Controller* a_observer) {
        observer = a_observer;
    }

private:
    Controller* observer = nullptr;
};

class Controller {
public:
    explicit Controller(std::unique_ptr<View> a_view) :
        view(std::move(a_view)) {
        view->SetObserver(this);
    }

private:
    std::unique_ptr<View> view;
};

int main() {
    Controller controller(std::make_unique<View>());
}

As a side note, you could use std::observer_ptr to signal your intent when it becomes part of the standard.

I didn't know about `std::observer_ptr`. Looks like a neat addition for future implementations. Thanks! — Mihai Todor, Mar 13 '16 at 05:55

Breaking a circular dependency between a shared_ptr and a unique_ptr

2 Answers2