Herb Sutter's 10-liner with cleanup

I've been adding data caching to my code and remembered "Herb Sutter's favorite 10-liner.":

shared_ptr<widget> get_widget(int id) {
    static map<int, weak_ptr<widget>> cache;
    static mutex m;

    lock_guard<mutex> hold(m);
    auto sp = cache[id].lock();
    if (!sp) cache[id] = sp = load_widget(id);
    return sp;
}

The one criticism I've seen of this is that, depending on context, it may be a problem that nothing is ever removed from the cache.

I was surprised I couldn't find the clean-up-after-itself solution anywhere. What do you think of this?

std::shared_ptr<widget> get_widget(int id) {
    static std::map<int, std::weak_ptr<widget>> cache;
    static std::mutex m;

    std::lock_guard<std::mutex> hold(m);
    auto& weakCached = cache[id]; // Keep a reference so we don't hae to call this non-nothrow function below.
    auto sp = weakCached.lock();
    if (!sp) {
        std::unique_ptr<widget> it = load_widget(id);
        // We have to be careful about the deleter not being
        // called while in the mutex lock 'cause that'd be a 
        // double lock => deadlock.
        // For that reason, we already have a 
        auto deleter = [&,id,d=it.get_deleter()](widget* w) {
            std::lock_guard<std::mutex> hold(m);
            d(w);
            cache.erase(id);
        };
        sp = std::shared_ptr<widget>(it.get(), deleter);
        it.release(); // In the case that the above line throws, we won't hit this line and so won't leak.
        weakCached = sp;
    }
    return sp;
}

As the comment says, the part that gives me pause is the transfer of ownership to the deleter function. In particular, the fact that calling it locks the mutex, so it can't be called while the mutex is locked. I think this correctly guarantees that it won't be called inside the critical section, particularly because if sp = std::shared_ptr<widget>(it.get(), deleter); throws, then it's still owned by the unique_ptr and the following line lets it go.

Bonus points: What if load_widget(int id) returns a std::shared_ptr<widget> which could already have a non-trivial deleter. Is there a way to inject this wrapper around it, or is that not possible?

Solution

I believe there is a simpler way to guarantee that the deleter won't run while hold holds the lock: Declare sp before hold, so when we go out of scope, the mutex will be released before the deleter can run:

std::shared_ptr<widget> sp;
std::lock_guard<std::mutex> hold(m);
auto& weakCached = cache[id];
sp = weakCached.lock();

It makes sense to reuse weakCached even without having to care about exception safety.

What if load_widget(int id) returns a std::shared_ptr<widget>

Here's an (wild?) idea:

std::shared_ptr<widget> it = load_widget(id);
widget* ptr = it.get();
auto deleter = [&,id,original=std::move(it)](widget*) {
    std::lock_guard<std::mutex> hold(m);
    cache.erase(id);
};
weakCached = sp = std::shared_ptr<widget>(ptr, std::move(deleter));

We keep a copy of the result from load_widget in the deleter. This causes our shared pointer to keep the original shared pointer alive. When our refcount is exhausted, our custom deleter doesn't do anything to the widget directly. The deleter is simply destroyed, and so the captured shared pointer is destroyed, and if it is the only owner, then the original custom deleter does its job.

This approach has a caveat: If a copy of the original shared pointer is somehow available elsewhere, then its lifetime may be extended beyond the lifetime of our shared pointer, and so the cache may be purged prematurely. Whether this is the case, and whether this is a problem depend on the widget API in question.

This approach can be used with your unique pointer case as well.