1

I am writing resource manager. That's how it looks like:

#pragma once class IObject; typedef std::shared_ptr<IObject> resource_ptr; typedef std::map<std::string, resource_ptr> resources_map; class ResourceManager { public: ResourceManager(void); ~ResourceManager(void); bool add(resource_ptr &resource); resource_ptr get(const std::string &name); void release(resource_ptr &ptr); private: resources_map resources; }; bool ResourceManager::add(resource_ptr &resource) { assert(resource != nullptr); resources_map::iterator it = resources.begin(); while(it != resources.end()) { if(it->second == resource) return false; it++; } resources[resource->getName()] = move(resource); return true; } resource_ptr ResourceManager::get(const std::string &name) { resources_map::iterator it = resources.find(name); resource_ptr ret = (it != resources.end()) ? it->second : nullptr; return ret; } void ResourceManager::release(resource_ptr &ptr) { assert(ptr); resources_map::iterator it = resources.begin(); while(it != resources.end()) { if(it->second == ptr) { ptr.reset(); if(!it->second) resources.erase(it); return; } it++; } }

And now, when I add new resource

 resource_ptr t = resource_ptr(new Texture(renderer)); t->setName("t1"); resourceManager.add(t);

The pointer has one reference. Now, when I want to get this pointer 

resource_ptr ptr = resourceManager.get("t1");

reference counter increases. So when I don't want to use this resource anymore

resourceManager.release(ptr);

I want to delete this resource at this moment but reference counter has value 1.

What should I do?

Improve this question

4 Answers 4

Reset to default
3

First, to directly answer your question, this is exactly what a weak_ptr is for.

It allows you to "watch" a reference-counted object, but without keeping it alive if the weak pointer is the only reference left.

Second, don't write manager classes. What do you need a "resource manager" for? What does "managing" resources entail? You store them as shared pointers, so they can pretty much manage themselves.

Any time you consider writing a "manager" class, you should stop and ask yourself "what is this class actually supposed to do?" And then rename it to something informative and specific. A "resource manager" could be anything and nothing. We know that it does... something with resources, but the name tells us nothing about what that something is. Is it an index allowing users to locate resources? Does it manage the lifetime of resources? Does it handle loading and unloading of resources? Or something entirely different? Or all of these things?

Decide on one thing that the class should do, then rename it so the name reflects that one thing.

Improve this answer
Sign up to request clarification or add additional context in comments.

3 Comments

Let's say that you have a program that reads in a bunch of images, and the same image is sometimes loaded more than once. Is a class that ensures that you only keep one copy of each image in memory not a reasonable use for resource manager? And if there is a better way to do this, what is it? (I have had this problem before, and the solution I came up with was a resource manager)
@Xymostech: It's not about the idea, it's about the name. What does ResourceManager tell you about the class? Nothing, really. ResourceCache or ResourceLocator, on the other hand, tell you everything the class is about.
@Xymostech: such a class might well be useful, but then give it a meaningful name. Now you have described what its responsibility is, and in telling me this, I understand what the purpose of the class is. But if, instead, you just tell me that "this is the resource manager", then it doesn't tell me anything. If you need such a class, then give it a name which explains what it does. Whenever you name a class "manager", it simply means "when I created this class, I didn't actually know what it was *for, and I'm still not really sure". Just give it a proper name.
3

As has been said, the pendant of std::shared_ptr is std::weak_ptr which allows to hold onto the resource without actually holding onto it. Therefore a trivial transformation would be to store a weak_ptr<T> as the value in your map to avoid artificially maintaining the object alive...

However there is an issue with this scheme: a space leak. The number of keys in your map will never decrease, meaning that if you load 1000 "resources" and release 999 of them, your map still has 1000 keys, 999 of them associated to a useless value!

The trick is, however, rather simple: upon destruction, the registered object should notify those which have a reference to it! This does impose a number of limitations though:

  • the name of the object should never change once it has been registered
  • an object should never been registered more than once and/or maintain a list of all those it is registered with

Finally, there is also the issue that those the object was registered to could die before the object does... Getting kinda complicated isn't it ?

So, here is our plan of attack:

  • What is only ever run once per instance ? The constructor.
  • How do you ensure liveness "passively" ? Using std::weak_ptr

Let's go!

// Object.hpp class Cache; class Object: public enable_shared_from_this<Object> { public: // std::shared_ptr<Object> shared_from_this(); -- inherited Object(std::string const& name, std::shared_ptr<Cache> const& cache); virtual ~Object(); Object(Object const&) = delete; Object& operator=(Object const&) = delete; std::string const& name() const { return _name; } private: std::string _name; std::weak_ptr<Cache> _cache; }; // class Object // Object.cpp #include <Object.hpp> #include <Cache.hpp> Object::Object(std::string const& name, std::shared_ptr<Cache> const& cache): _name(name), _cache(cache) { if (cache) { cache->add(this->shared_from_this()); } } Object::~Object() { std::shared_ptr<Cache> c = _cache.lock(); if (c) { c->release(*this); } }

A couple of weird things going on here:

  • by passing the name in the constructor we guarantee that it's set, and since we don't provide any setter it cannot be modified either (unless const_cast...)
  • inheriting from enable_shared_from_this means that if the object's lifetime is managed by a shared_ptr then using shared_from_this we can get a shared_ptr pointer to it
  • we have to be careful that we do have a reference to a still alive cache in the destructor, so we check for it.
  • let's use a virtual destructor, just to be on the same side.

Okay, so let's go on:

// Cache.hpp #include <Object.hpp> class Cache: public std::enable_shared_from_this<Cache> { friend class Object; public: // std::shared_ptr<Cache> shared_from_this(); -- inherited std::shared_ptr<Object> get(std::string const& name) const; void release(Object const& o); private: typedef std::weak_ptr<Object> WeakPtr; typedef std::map<std::string, WeakPtr> Map; void add(std::shared_ptr<Object> const& p); Map _map; }; // class Cache // Cache.cpp #include <Cache.hpp> std::shared_ptr<Object> Cache::get(std::string const& name) const { auto const it = _map.find(name); if (it == _map.end()) { return std::shared_ptr<Object>(); } return it->second.lock(); } void Cache::release(Object const& o) { _map.erase(o.name()); } void Cache::add(std::shared_ptr<Object> const& p) { assert(p && "Uh ? Should only be accessed by Object's constuctor!"); _map[p->name()] = p; // Note: override previous resource of same name, if any }

This seems pretty easy now. Usage:

int main() { std::shared_ptr<Cache> c{new Cache{}}; // cannot be stack allocated no longer { std::shared_ptr<Object> o{new Object{"foo", c}}; assert(c->get("foo")); } assert(c->get("foo") == nullptr); std::shared_ptr<Object> o{new Object{"foo", c}}; c.reset(); // destroy cache // no crash here, we just do not "unregister" the object }
Improve this answer

Comments

1

Here is a really simple resource manager that uses weak_ptr and shared_ptr.

template<typename T, typename Arg=std::string, typename Ordering = std::less<Arg>> class ResourceManager { typedef std::function<std::shared_ptr<T>(Arg)> factory; factory createT; std::map< Arg, std::weak_ptr<T>, Ordering > cache; public: ResourceManager( factory creator ):createT(creator) {} std::shared_ptr<T> get( Arg a ) { std::shared_ptr<T> retval; auto it = cache.find(a); if (it != cache.end()) { retval = it->second.lock(); } if (retval) return retval; retval = createT(a); cache[a] = retval; return std::move(retval); } };

Now, this requires that you can create a resource from its name, or more specifically that the name (Arg) fully specifies the resource, and whenever you ask for the resource you are ok with it being constructed.

Write a function that takes a filename std::string and returns a loaded image and pass it to a ResourceManager< image > constructor, and get images by calling manager.get(string), and the above should work. In a multi-threaded environment things get trickier, naturally.

The get() code can be optimized through use of equal_range (to give a hint to the insert afterwards -- no need to search the map twice) or unordered maps (because you don't care about the map ordering), etc. Code has not been compiled.

Sample use:

void DisposeImage( Image* img ); // TODO: write Image* LoadImage( std::string s ); // TODO: write shared_ptr<Image> ImageFactory( std::string s ) { return shared_ptr<Image>( LoadImage(s), DisposeImage ); } ResourceManager manager( ImageFactory ); std::shared_ptr<Image> bob1 = manager.get("Bob.png"); std::shared_ptr<Image> doug1 = manager.get("Doug.png"); std::shared_ptr<Image> bob2 = manager.get("Bob.png"); Assert(bob1.get() == bob2.get());
Improve this answer

1 Comment

Not much of a problem, if it is one. Make the Manager a shared_from_this, pass in both a creator of a T* and a destroyer of a T* to the Manager, have it wrap the shared_ptr with a cleanup function that calls both unregister and the destroy on a shared-pointer copy of this, and create a static std::shared_ptr CreateInstance() in the manager. As a side effect, this will also prevent misses from being cached. Now, for a real image cache, you'll also have to deal with the possibility of the file on disk being changed, and orphan the old cache entry and outstanding shared_ptrs.
0

Smart pointers are used to automatically control an object's lifetime. In this case it looks like you don't want automatic control, you want to control it explicitly. So don't use a smart pointer.

Improve this answer

Comments

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.