C++ Software cache for shared resources I wrote a basic C++ template to manage shared resources like textures and shaders in a 3D engine. The idea is that the cache itself holds weak references to the resources (through std::weak_ptr) and turns it into strong references (through std::shared_ptr) when the resource is fetched from the cache or constructed if it's not present. As soon as all strong references go out of scope, the given object gets deleted and removed from the cache by a custom deleter. Suggestions/improvements are welcome:



#ifndef PTRCACHE_H
#define PTRCACHE_H

#include &lt;memory&gt;
#include &lt;map&gt;
#include &lt;functional&gt;
#include &lt;mutex&gt;

namespace MyEngine
{
  template&lt;typename Key, typename Val, typename ... Args&gt;
  class PtrCache
  {
  public:
    PtrCache(const std::function&lt;Val*(Args...)&gt;&amp; creator) : 
      _creator(creator),
      _mutex(std::make_unique&lt;std::mutex&gt;())
    {}
    PtrCache(const PtrCache&amp; other) = delete;
    PtrCache&amp; operator=(const PtrCache&amp; other) = delete;
    PtrCache(PtrCache&amp;&amp; other) :
      _cache(std::move(other._cache)),
      _creator(std::move(other._creator)),
      _mutex(std::move(other._mutex))
    {}
    PtrCache&amp; operator=(PtrCache&amp;&amp; other)
    {
      if (this != &amp;other) {
        _cache = std::move(other._cache);
        _creator = std::move(other._creator);
        _mutex = std::move(other._mutex);
      }
      return *this;
    }
    /**
    * Returns true if ret was taken from the cache, false otherwise.
    */
    bool getOrCreate(const Key&amp; key, std::shared_ptr&lt;Val&gt;&amp; ret, Args... args)
    {
      std::lock_guard&lt;std::mutex&gt; lock(*_mutex);
      auto it = _cache.find(key);
      if (it != _cache.end()) {
        ret = std::shared_ptr&lt;Val&gt;(it-&gt;second); // Construct from weak_ptr
        return true;
      }
      ret = std::shared_ptr&lt;Val&gt;(_creator(args...), [this, key](Val* ptr) { // Construct from creator and pass custom deleter which removes the element from the cache.
        std::lock_guard&lt;std::mutex&gt; lock(*_mutex);
        _cache.erase(key);
        delete ptr;
      });
      _cache[key] = ret;
      return false;
    }
    inline void clear()
    {
      std::lock_guard&lt;std::mutex&gt; lock(*_mutex);
      _cache.clear();
    }
    inline size_t size()
    {
      std::lock_guard&lt;std::mutex&gt; lock(*_mutex);
      return _cache.size();
    }
  private:
    std::map&lt;Key, std::weak_ptr&lt;Val&gt;&gt; _cache;
    std::function&lt;Val*(Args...)&gt; _creator;
    std::unique_ptr&lt;std::mutex&gt; _mutex;
  };
}

#endif


Basic usage example:



PtrCache&lt;std::string, GLTexture, const std::string&amp;&gt; texture_cache([] (const std::string&amp; path) {
  auto tex = SOIL_load_OGL_texture(path.c_str(), SOIL_LOAD_AUTO, SOIL_CREATE_NEW_ID, SOIL_FLAG_MIPMAPS | SOIL_FLAG_TEXTURE_REPEATS | SOIL_FLAG_COMPRESS_TO_DXT);
  if (tex) {
    return new GLTexture(tex, GL_TEXTURE_2D);
  }
  else {
    throw std::exception((std::string("Could not create texture ") + path).c_str());
    return nullptr;
  }
});

std::shared_ptr&lt;GLTexture&gt; my_texture;
std::string path = "my_texture.png";
texture_cache.getOrCreate(path, my_texture, path);


The only caveat I see is that the user of this class has to ensure that the lifetime of the cache goes beyond the lifetime of any contained managed object. Otherwise, the custom deleter would obviously cause undefined behaviour.
The basic idea of your cache, as I understand it, is that it is a map of weak pointers connected to shared pointers with custom deleters that remove them from the map. As you noticed yourself, that's a risky pattern, because if any resources outlive the cache, you get UB.

Is it really necessary for objects to remove themselves from the cache? What if you just let them destroy themselves naturally, and leave the empty weak pointer in the cache. Then your lifetime worries fade away. From time to time you can purge the cache of entries where expired() == true if you really want, and other functions of the cache will have to take into account expired entries (like size() for example). It even makes your cache more flexible, because if you're not using the deleter you allow the possibility of users providing their own.

I also question the wisdom of keeping the "creator" function object in the cache, for a couple of reasons:


It makes the cache itself so much clunkier to use, because you have to specify all the arguments to the create function in the template parameter list.
It makes the cache less flexible, because there may be multiple ways to create (for example) a texture (such as from a file, or from a memory buffer), so you might want different create function signatures.


Rather than hard-coding the create function's signature in the type, and deciding on the create function for everything in cache at cache construction time, why not just pass an optional create function when and where it's needed?

I'll bring these points up again as I go through the code.

PtrCache(const std::function&lt;Val*(Args...)&gt;&amp; creator) : 
  _creator(creator),
  _mutex(std::make_unique&lt;std::mutex&gt;())
{}


Single-argument constructors should pretty much always be explicit.

Since this is a pure sink function, you should really take the arguments by value, not by const&amp;. And then you should move them into their final place:

explicit PtrCache(std::function&lt;Val*(Args...)&gt; creator) : 
  _creator{std::move(creator)},
  _mutex{std::make_unique&lt;std::mutex&gt;()}
{}


But I would suggest not having the _creator member variable, so you don't need this constructor at all. Or the Args... template parameter for the class either.

PtrCache(PtrCache&amp;&amp; other) :
  _cache(std::move(other._cache)),
  _creator(std::move(other._creator)),
  _mutex(std::move(other._mutex))
{}
PtrCache&amp; operator=(PtrCache&amp;&amp; other)
{
  if (this != &amp;other) {
    _cache = std::move(other._cache);
    _creator = std::move(other._creator);
    _mutex = std::move(other._mutex);
  }
  return *this;
}


There's no need to write out either of these, because they're just the default implementations. You can just do:

PtrCache(PtrCache&amp;&amp; other) noexcept = default;
PtrCache&amp; operator=(PtrCache&amp;&amp; other) noexcept = default;


Also, both can be noexcept (and move ops always should be).

bool getOrCreate(const Key&amp; key, std::shared_ptr&lt;Val&gt;&amp; ret, Args... args)
{
  std::lock_guard&lt;std::mutex&gt; lock(*_mutex);
  auto it = _cache.find(key);
  if (it != _cache.end()) {
    ret = std::shared_ptr&lt;Val&gt;(it-&gt;second); // Construct from weak_ptr
    return true;
  }
  ret = std::shared_ptr&lt;Val&gt;(_creator(args...), [this, key](Val* ptr) { // Construct from creator and pass custom deleter which removes the element from the cache.
    std::lock_guard&lt;std::mutex&gt; lock(*_mutex);
    _cache.erase(key);
    delete ptr;
  });
  _cache[key] = ret;
  return false;
}


This is the meat of the cache, and so the best place to talk about broader problems. But the function has plenty of problems of its own.

First, out-arguments are a bad idea. They're clunky as all hell. Every use of this function - which is the main function of the cache - requires two lines:

std::shared_ptr&lt;GLTexture&gt; result;
texture_cache.getOrCreate("my_texture.png"s, result, "my_texture.png"s);


The natural place for the output of a function is the return value. If you want multiple return values - so you can get that bool as well - use a custom return struct, or a tuple:

std::tuple&lt;std::shared_ptr&lt;Val&gt;, bool&gt; getOrCreate(const Key&amp; key, Args... args);


Now this is much easier to use, especially since C++17:

auto [ result, created ] = texture_cache.getOrCreate("my_texture.png"s, my_texture.png"s);


At a higher level, you've hard-coded the create function into the cache, which makes it less flexible. Rather than doing that, you can take a function object in getOrCreate(), like this:

auto [ tex1, created1 ] = texture_cache.getOrCreate("texture_1", [] { return load_texture_from_file("texture_1.png"); });
auto [ tex2, created2 ] = texture_cache.getOrCreate("texture_2", [] { return generate_procedural_texture(seed_value); });
// and so on


The signature of getOrCreate() now becomes:

template &lt;typename Func&gt;
std::tuple&lt;std::shared_ptr&lt;Val&gt;, bool&gt; getOrCreate(const Key&amp; key, Func&amp;&amp; func);


and the creation line in the function is:

ret = std::make_shared&lt;Val&gt;(func());


Or even better, let the cache objects construct themselves:

template &lt;typename Args...&gt;
std::tuple&lt;std::shared_ptr&lt;Val&gt;, bool&gt; getOrCreate(const Key&amp; key, Args&amp;&amp;... args)
{
    // ...
    ret = std::make_shared&lt;Val&gt;(std::forward&lt;Args&gt;(args)...);
    // ...
}

// used as:
auto [ tex1, created1 ] = texture_cache.getOrCreate("texture_1", from_file, "texture_1.png");
auto [ tex2, created2 ] = texture_cache.getOrCreate("texture_2", noise_texture, seed_value);
// assumes texture_cache holds "texture" objects that have the following constructors, or something similar:
// texture::texture(from_file_t, std::filesystem::path);
// texture::texture(noise_texture_t, std::uint_fast32_t);


There are multiple ways to do all this that are way more flexible than setting the create function and its signature when the cache is created.

This function also seems a little over-complex, using find() and then []. Your map values are weak pointers. There's really no need for all that dancing around to get-or-set them in the map. You can just do:

bool getOrCreate(const Key&amp; key, std::shared_ptr&lt;Val&gt;&amp; ret, Args... args)
{
    std::lock_guard&lt;std::mutex&gt; lock(*_mutex);
    auto&amp; wkptr = _cache[key];
    ret = wkptr.lock();
    if (ret)
        return true;
    // Either the value wasn't in the map, or it was but it expired
    // (so it was effectively removed from the map). Either way,
    // make a new thing and store it in the cache.
    ret = std::make_shared&lt;Val&gt;(_creator(args...));
    wkptr = ret;
    return false;
}


Also, along with everything I mentioned above about the create function stuff, if you're going to make this function a template - either to take a create function object or an argument parameter pack to pass to the value type's constructor - you might as well take advantage of that and use perfect forwarding for the key as well.

Also-also, you use make_unique(), and that's good... but it's not that important. It is, however, important to use make_shared(). You should aim for that in the function's design.

inline void clear()
{
    std::lock_guard&lt;std::mutex&gt; lock(*_mutex);
    _cache.clear();
}


The inline keyword serves no purpose here. Functions defined in the class are inline by default. What would be handy here are const and noexcept. At least const, because locking might throw an error. Same goes for size().

There's another nasty little issue with having a clear() function, though. Your cache objects erase themselves from the cache when the last one goes out of scope, right? Consider this scenario:


Subsystem A creates a cache object with key "foo", and gets an instance.
Subsystem (any!) calls clear() on the cache. Subsystem A's "foo" instance is still valid, of course. It just doesn't have an entry in the cache anymore.
Subsystem B creates a new cache object with the key "foo", and gets an instance.
Subsystem A lets its "foo" instance expire... which triggers the deleter to erase "foo" from the cache.
Subsystem B asks for another "foo" object from the cache, expecting to get an alias of the one it got in step 3 because it knows no-one's called clear() since it got the "foo" object in step 3... instead it gets a whole new "foo".


In other words, the cache becomes unpredictable. Even if I'm holding an object I got from a specified key and I know for sure no one's cleared the cache since I got it, I still can't be sure I'll get the same object from the same key (or any object at all from that key).

std::map&lt;Key, std::weak_ptr&lt;Val&gt;&gt; _cache;


Is there any reason you're not using an unordered map here?

Summary


As you've noted, having cache objects remove themselves from the cache when they expire invites problems. There's no good reason to do that. Just leave the empty weak pointers in the cache, and cull them from time to time (or not; just leave them there and ignore them).
Defining the create function arguments at instantiation time, and the create function itself at construction time, is inflexible, and it complicates the class. It's much more flexible to be able define creation parameters when and where I need them. The class should be PtrCache&lt;Key, Val&gt;, maybe with an allocator option. Having Args... in there is clunky and restrictive.
The most flexible option is probably to let your cache objects' constructors do what constructors are for, and define getOrCreate() as a template function taking arguments that get perfect-forwarded to the constructor if and only if something actually gets constructed.
Use make_shared().
Don't manually redefine implicitly generated functions; just = default them.