Of common problems with shared pointers

There’s one repeating pattern in all C++ code bases I’ve worked with. shared_ptr is abused - one way or another. There are many reasons, sometimes people just start with shared_ptr instead of unique_ptr out of laziness, sometimes it’s the sole, default smart pointer they rely on. Often, it’s a result of many passes of refactoring and eventual quality degradation with time. This leads to all sorts of problems but there’s are definitely some repeating patterns.

Circular references

We all know (I hope) about the classic cyclic reference problem that often arises when two objects both mutually own themselves and refer to themselves at the same time. Something along the lines:

 1
 2
 3
 4
 5
 6
 7
 8
 9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33

class B;

class A {
public:
    void setB(std::shared_ptr<B> b) {
        b_ = b;
    }

    ~A() {
        std::cout << "A destructor" << std::endl;
    }

private:
    std::shared_ptr<B> b_;
};

class B {
public:
    explicit B(std::shared_ptr<A> a) : a_(a) {}

    ~B() {
        std::cout << "B destructor" << std::endl;
    }

private:
    std::shared_ptr<A> a_;
};

int main() {
    auto a = std::make_shared<A>();
    auto b = std::make_shared<B>(a);
    a->setB(b);
}

This, of course, inevitably leads to a memory leak as the reference count never drops to zero for both of them.

I’ve seen that often in failed attempts to implement pimpl pattern or the observer pattern. The easiest way to break the circular reference is to use std::weak_ptr instead. The above example can be fixed by changing the A::b_ to be a std::weak_ptr:

 1
 2
 3
 4
 5
 6
 7
 8
 9
10
11
12
13

class A {
public:
    void setB(std::shared_ptr<B> b) {
        b_ = b;
    }

    ~A() {
        std::cout << "A destructor" << std::endl;
    }

private:
    std::weak_ptr<B> b_;
};

Of course, it’s never as simple and plain as in the example above. Most of the time, the ownership hierarchy involves multiple objects creating the reference cycle. The underlying root-cause is the same.

The introduction of smart pointers brought some sort of a general distaste an apprehension towards raw pointers and references as well but this is completely unjustified in my opinion. Raw pointers are perfectly fine to express an interest in a resource but lack of participation in ownership - they shouldn’t be disregarded, treated as legacy or anything like that.

Unpredictable point of destruction

By definition, when using a shared pointer, we are expressing that a given object’s ownership is shared between multiple owners. Natural, well understood consequence of this is that any of the owners is extending the lifetime of the object.

The other implication is that the object’s destruction point is not predictable. The order in which the shared pointers are destroyed might be undefined or dependent on external factors, like e.g. timing. As a result, none of the owners can assume that by dropping their instance of the pointer, the managed object will indeed get destroyed. The act of resetting the shared_ptr merely expresses owner’s termination of participation in shared ownership.

What if I hold an instance of shared_ptr and I want to be sure that the destruction won’t happen if I drop it? This is an interesting, real world problem, something that Timur Doumler has quite well discussed in his talk about C++ in the audio industry:

Long story short, dropping shared_ptr on a real-time audio thread might break real-time guarantees therefore it’s unacceptable and must be prevented.

He’s presenting an interesting idea of a ReleasePool. Objects are added to ReleasePool prior to being used on high priority thread - therefore it’s guaranteed that once the shared_ptr is dropped there, the destruction won’t happen.

The ReleasePool is polled and cleaned on a low priority thread so, it kind of emulates the garbage collection mechanism.
Here’s the relevant parts from his presentation.

 1
 2
 3
 4
 5
 6
 7
 8
 9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45

class ReleasePool : private Timer
{
public:
    ReleasePool() { startTimer (1000); }

    template<typename T> 
    void add (const std::shared_ptr<T>& object) {
        if (object.empty())
            return;
        std::lock_guard<std::mutex> lock (m);
        pool.emplace_back (object);
    }

private:
    void timerCallback() override {
        std::lock_guard<std::mutex> lock (m);
        pool.erase(
            std::remove_if(
                pool.begin(), pool.end(),
                [] (auto& object) { return object.use_count() <= 1; } ),
                pool.end());
    }
    std::vector<std::shared_ptr<void>> pool;
    std::mutex m;
};

// later on used the following way:

class Synthesiser
{
public:
    void audioCallback (float* buffer, int bufferSize) {
        std::shared_ptr<Widget> widgetToUse = std::atomic_load (&currentWidget);
        // do something with widgetToUse...
    }

    void updateWidget ( /* args */ ) {
        std::shared_ptr<Widget> newWidget = std::make_shared<Widget> ( /* args */ );
        releasePool.add (newWidget);
        std::atomic_store (&currentWidget, newWidget);
    }

    std::shared_ptr<Widget> currentWidget;
    ReleasePool releasePool;
};

Shared pointers and threads

shared_ptr ownership and threads often lead to a problem that I refer to as “ownership inversion”. What do I mean by that? Have a look at the following example:

 1
 2
 3
 4
 5
 6
 7
 8
 9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61

class Timer {
public:
    Timer() : t_{[this] (std::stop_token st) mutable { 
        std::unique_lock<std::mutex> l(m_);
        cv_.wait(l, st, [this] { return static_cast<bool>(f_); });
        auto f = std::move(f_);
        std::this_thread::sleep_for(delay_);
        f();
        std::cout << "Thread exiting" << std::endl;
    }}
    {
    }

    ~Timer() {
        std::cout << "Timer destructor" << std::endl;
    }

    void schedule(std::function<void()> f, std::chrono::seconds delay) {
        std::unique_lock<std::mutex> l(m_);
        f_ = std::move(f);
        delay_ = delay;
        cv_.notify_one();
    }

private:
    std::jthread t_;
    std::function<void()> f_;
    std::chrono::seconds delay_;
    std::mutex m_;
    std::condition_variable_any cv_;
};


class A : public std::enable_shared_from_this<A> {
public:
    explicit A(std::shared_ptr<Timer> t) : d_{std::move(t)} 
    {
    }

    void schedule() {
        d_->schedule([self = shared_from_this()] { self->hello(); }, std::chrono::seconds(2));
    }

    void hello() {
        std::cout << "Hello, world!" << std::endl;
    }

private:
    std::shared_ptr<Timer> d_;
};

int main() {
    auto t = std::make_shared<Timer>();
    auto a = std::make_shared<A>(std::move(t));

    a->schedule();
    a.reset();

    std::this_thread::sleep_for(std::chrono::seconds(5));
    return 0;
}

What’s gonna happen if I run this? The expected result is that it should print “Hello, world!” after two seconds and terminate after three (-ish) more. But here’s what it does:

1
2
3
4
5
6
7

$ ./a.out 
Hello, world!
Thread exiting
Timer destructor
terminate called after throwing an instance of 'std::system_error'
  what():  Resource deadlock avoided
Aborted (core dumped)

A owns the Timer which owns the thread t_. The functor scheduled on the timer is the only owner of A once it executes, both A and Timer are being destructed which leads to an attempt to join thread t_ from itself - resulting in an exception being thrown as described in the std::jthread documentation:

resource_deadlock_would_occur if this->get_id() == std::this_thread::get_id() (deadlock detected).

In other words, the ownership has been transferred from the main thread to the Timer thread.

How to address that? std::weak_ptr is merely a mitigation. Once locked on Timer thread, the main thread might drop all its shared_ptr instances resulting in this problem back again.

The problem is in the ownership model and the lifetime of the objects which is incorrect. In this example, the lifetime of A on main thread has to extend beyond anything that’s happening on Timer thread.

These kind of bugs are unexpected and initially, difficult to spot by just looking at the code yet very common and indicative of bad ownership model, most of the time, caused by over reliance on shared_ptrs.

Conclusion

Ownership model is an important part of software design. Smart pointers are not just simple wrappers for managing resources but define objects lifetime, the dependency graph and ownership model between participating objects and modules.

As mentioned in C++ core guidelines one should prefer unique_ptr whenever possible before considering shared_ptr. This is the rule of thumb I try to follow and hopefully, with the examples I’ve discussed here, I’ve managed to convince you to do the same.