0

My application starts several similar processes. First one creates a "global" temp file that they all red/write. When last process is destroyed, this file needs to be deleted. Later more processes may be spun up and this file should be re-created. From various examples I came up with a manager class that creates shared_ptr's with custom deleter (removing the file) and holds week_ptr to these objects to be able to hand them out upon request. This works but I'm wondering if there is a better approach, or any pitfalls can be spotted in my code.

#include <iostream>
#include <memory>
#include <string>
#include <set>

bool ptr_less(std::weak_ptr<std::string> lhs, std::weak_ptr<std::string> rhs) {
    auto left = lhs.lock();
    auto right= rhs.lock();
    if (!left || !right)
        return left < right;
    return *left < *right;
}

struct Manager {
    std::shared_ptr<std::string> get(const std::string& filename) {
        auto result = resources.find( std::weak_ptr<std::string>(std::make_shared<std::string>(filename)) );
        if (result != resources.end()) {
            std::cout << "Exists: " << filename << std::endl;
            if (auto sp = result->lock())
                return sp;
            resources.erase(result);            
        }
        // Create new object to manage, auto deleting
        std::shared_ptr<std::string> ptr(new std::string(filename), 
          [](std::string* str) { std::cout << "remove: " << *str << std::endl; delete str; });
        resources.emplace(std::weak_ptr<std::string>(ptr));
        //cleanup null pointers
        for (auto iter=resources.begin(); iter != resources.end(); ) {
            if (!iter->lock())
                iter = resources.erase(iter);
            else
                ++iter;
        }
        return ptr;
    }
    //Keep track of files to share. std::set comparing values not pointers
    std::set<std::weak_ptr<std::string>, decltype(ptr_less)*> resources{ptr_less};
};
static Manager custodian;

struct User {
    User(std::string name) : mName(std::move(name)) {
        std::cout << "New user: " << mName << std::endl;
        mGlob = custodian.get("global.json");
        mSet  = custodian.get("settings-" + mName + ".json");
        mVal  = custodian.get("values-"   + mName + ".json");
    }
    ~User() {
        std::cout << "~User(): " << mName << std::endl;
    }
    std::shared_ptr<std::string> mGlob;
    std::shared_ptr<std::string> mSet;    
    std::shared_ptr<std::string> mVal;    
    std::string mName;
};

int main()
{
    using namespace std;
    User* ps3 { nullptr };
    {
        User ps1("ps1");
        User ps2("ps2");
        ps3 = new User("ps3");
    }
    delete ps3;
    cout << "Resources size: " << custodian.resources.size() << endl;
    User ps1("ps1");
    cout << "Resources size: " << custodian.resources.size() << endl;
    return 0;
}

Demo here

L8Cod3r
  • 60
  • 6
  • It looks like you only keep weak pointers in the `resources` set. This will not keep the objects alive. – Richard Critten Sep 23 '21 at 16:35
  • @RichardCritten `resources` keeps week pointers intentionally, so I don't keep allocated objects past their usefulness. if `result->lock()` fails I intend to fall through the `}` and create a new object. – L8Cod3r Sep 23 '21 at 16:42
  • process is pretty different from thread (you're not using any in the example btw), you cannot even use `weak_ptr` cross process border. How can it *work*? – apple apple Sep 23 '21 at 17:11
  • The implementation provided here doesn't make much sense; if the manager doesn't have a given resource, it instantiates one and returns that while holding a `weak_ptr` to it. If something else comes and asks for that same resource, it'll hand back a `shared_ptr` to it and then stop holding the `weak_ptr`, which means that if there's a third request for that resource, it'll instantiate yet another new object for a resource which might, in fact, be in use already. – Kyle Sep 23 '21 at 17:16
  • Another problem here is that you have to do a whole temporary heap allocation just to do the lookup. This could be avoided by using a transparent comparator. – Kyle Sep 23 '21 at 17:17
  • @Kyle I appreciate your replies. When handing back a 2nd `shared_ptr` I still hold the week_ptr. It's removed only when `lock()` returns `nullptr`, meaning it's no longer used by anyone. This is c++11 I wasn't aware of `transparent comparator`, will research. – L8Cod3r Sep 23 '21 at 17:30
  • @appleapple this is just a demo to show resource management. The processes I'm referring to only use file names and don't share any memory. – L8Cod3r Sep 23 '21 at 17:33
  • @L8Cod3r please provide real (at least relevant) code, what you showed cannot work multi-process so there is nothing we can answer here that is useful to your real situation. – apple apple Sep 23 '21 at 18:34

1 Answers1

0

weak_ptr is probably the wrong tool for the job here.

Consider this line:

 auto result = resources.find( std::weak_ptr<std::string>(std::make_shared<std::string>(filename)) );
    

What happens here is you create a copy of filename on the heap and manage it via a newly created shared_ptr. You then construct a weak_ptr, after which the shared_ptr goes out of scope, leaving you with an empty weak_ptr. So you could have just started out with an empty weak_ptr in the first place.

As mentioned in the comments, shared_ptr is not an inter-proces communication technique. If you want to synchronize resource across multiple processes (which it seems you want to be doing), you will need proper IPC mechanisms. The C++ Standard Library does not provide those, so you will have to look at other libraries for this purpose.

Also note that your ptr_less function is not doing valid things. In C++, you are only allowed to compare certain related values using operator <.

ComicSansMS
  • 51,484
  • 14
  • 155
  • 166