shared_ptr is getting deleted early

Question

I modified the code used in @Timo's answer to try to understand how shared_ptr and custom deleter works.

Here is the link to new code, or right here:

#include <memory>
#include <vector>
#include <iostream>
#include <string>

class TopicPointer
{
    public:
     TopicPointer(int x) : _x(std::move(x))
     {

     }

     ~TopicPointer(){
         std::cout << "Deleting " << _x <<  std::endl;
     }
     int GetX()
     {
         return _x;
     }
     private:
        int _x;

};

class Topic
{
    std::string name;
    std::shared_ptr<TopicPointer> _topicPointer;

    public:
        Topic(std::string name,std::shared_ptr<TopicPointer> topicPointer) : name(move(name)), _topicPointer(std::move(topicPointer)) {}

        ~Topic(){
            std::cout << "Deleting " << name << std::endl;
        }
};

struct Deleter
{
    public:
     void operator()(TopicPointer* ptr)
     {
        std::cout << "deleting topic " << ptr->GetX() << '\n';
     }
};

class TopicsCache 
{
public:
    std::unique_ptr<Topic>&& createTopic(std::string name, int y)    
    {
        auto topicPtr = new TopicPointer(y);
        return std::move(std::unique_ptr<Topic>(new Topic(move(name),std::shared_ptr<TopicPointer>(topicPtr, Deleter()))));
    }


};

class Subject
{
    public:
     Subject(std::vector<std::unique_ptr<Topic>> &&topics)  : _topics (std::move(topics))
     {

     }

    private:
      std::vector<std::unique_ptr<Topic>> _topics;
};

TopicsCache cache;

Subject BuildSubject()
{
    std::vector<std::unique_ptr<Topic>> topics;
    std::cout << "Creating topic 1\n";
    topics.emplace_back(std::move(cache.createTopic("a",1)));    
    std::cout << "Created topic 1\n";
    std::cout << "Creating topic 2\n";
    topics.emplace_back(std::move(cache.createTopic("b",2)));
    std::cout << "Created topic 2\n";
    topics.emplace_back(std::move(cache.createTopic("c",3)));
    topics.emplace_back(std::move(cache.createTopic("d",4)));
    return  Subject(std::move(topics));
}

int main()
{
    Subject subject = BuildSubject();

    std::cout << "Done";

}

As you can see, from the outpu of BuildSubject(): Creating topic 1

Deleting a

deleting topic 1

Initialized temp 1

Created topic 1

Creating topic 2

Deleting b

deleting topic 2

Created topic 2

Deleting c

deleting topic 3

Deleting d

The shared_ptr are deleted before the initialization to temp variable.

I thought when a shared_ptr is copied the reference count is updated? Also doesn't std::move preserve the reference count?

How to stop the shared_ptr from getting disposed early?

Thanks

Sorry, but I will not follow external links to get an idea what the question is about. And external links may be invalid soem days ahead and the whole content of your question is useless. — Klaus, Aug 11 '20 at 07:15
@Klaus it's a huge piece of code but I will modify the question — Lews Therin, Aug 11 '20 at 07:16
Try and keep your question self-contained, don't just jam in a bunch of links to other things. — tadman, Aug 11 '20 at 07:16
If your example code is to big, reduce it to a minimal compilable example please! — Klaus, Aug 11 '20 at 07:16
My compiler is right away saying `: In member function ‘std::unique_ptr&& TopicsCache::createTopic(std::string, int)’: :51:25: warning: returning reference to temporary [-Wreturn-local-addr]`. Returning a `std::unique_ptr&&` doesn't make much sense. — KamilCuk, Aug 11 '20 at 07:35
@KamilCuk I was able to fix up the bad pointer. But the shared_ptrs are still deleted too early. — Lews Therin, Aug 11 '20 at 07:38
I thought shared_ptr reference counts are incremented when copied.. so not sure why deleted so early — Lews Therin, Aug 11 '20 at 07:39
Provide some [mre] and learn to use [valgrind](https://valgrind.org/) — Basile Starynkevitch, Aug 11 '20 at 07:39
`too early.` What does it mean "too early"? "Too early" as compared to what? When did you expect/want then to be deleted? — KamilCuk, Aug 11 '20 at 07:39
@BasileStarynkevitch I pasted a link to a compilable version and got downvoted for it. Not sure what else you expect from me? — Lews Therin, Aug 11 '20 at 07:40
@KamilCuk They are destructed before "Created topic" i.e. they aren't destructed after "Done" — Lews Therin, Aug 11 '20 at 07:41
[GCC](http://gcc.gnu.org) is a [free software](https://www.gnu.org/philosophy/free-sw.en.html) C++ compiler and implementation, so **download its source code and study it** (budget several weeks of efforts). C++ is a very complex programming language, did you read [this book](https://www.stroustrup.com/programming.html), [this reference](https://en.cppreference.com/w/cpp) then [n3337](http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2012/n3337.pdf) - a C++ standard ? — Basile Starynkevitch, Aug 11 '20 at 07:42
And you are measuring the order of destructors by the output of your program? Please update your source with modifications you did that you were "able to fix up the bad pointer" and please post the output of your program. Stackoverflow is a bad place to learn programming. I did changed `std::unique_ptr&&` to `std::unique_ptr` and I see `Deleting something` lines written after `Done`. — KamilCuk, Aug 11 '20 at 07:43
See also [this answer](https://stackoverflow.com/a/63352529/841108) and the references there... — Basile Starynkevitch, Aug 11 '20 at 07:47
`createTopic` returns a reference to a local variable resulting in undefined behaviour, if you remove the unnecessary `std::move` the compiler will give you the relevant warning: https://godbolt.org/z/o6hcsr — Alan Birtles, Aug 11 '20 at 07:50

Dmitry Kuzminov · Accepted Answer · 2020-08-11T07:55:08.590

1

I see the problem here:

std::unique_ptr<Topic>&& createTopic(std::string name, int y)    
{
    auto topicPtr = new TopicPointer(y);
    return std::move(std::unique_ptr<Topic>(new Topic(move(name),std::shared_ptr<TopicPointer>(topicPtr, Deleter()))));
}

You are returning the reference to the temporary object that wouldn't be valid on the time you are using it:

topics.emplace_back(std::move(cache.createTopic("a",1)));

You should return a value instead:

std::unique_ptr<Topic> createTopic(std::string name, int y)    
{
    auto topicPtr = new TopicPointer(y);
    return std::unique_ptr<Topic>(new Topic(move(name),std::shared_ptr<TopicPointer>(topicPtr, Deleter())));
}

Overall you have plenty of problems in your code. Moving integers? Having the deleter that doesn't delete? Having parameters of type rvalue reference (sink pattern)? Creating a raw pointer and passing it into the std::shared_ptr constructor on the next line? You are ignoring all the idioms of C++ and you would shot into your leg soon.

edited Aug 11 '20 at 07:55

answered Aug 11 '20 at 07:49

Dmitry Kuzminov

6,180
6
18
40

Thanks, that seemed to do the trick. Moving integers was just a contrived example. It's irrelevant I just needed something to print. Why is the sink pattern bad? I need to create a raw pointer and passing into a shared_ptr because in the code I'm refactoring the Deleter needs to hold a pointer to the object to do some clean up before deleting the object. shared_ptr doesn't seem to be able to pass the address of the created type into a Deleter. But I agree it's a mess, which is why I stayed a way from C++ :( – Lews Therin Aug 11 '20 at 08:02
@LewsTherin, every pattern has it's own applicability. Sink pattern is useful in very rare cases (BTW, the deleter is one of them), but it shouldn't be used to initialize the `class Subject`. Raw pointers shouldn't be used like you did: that is not exception-safe. `std::make_unique` or `std::make_shared` is a solution. Otherwise you have to put this into `try/catch` block. – Dmitry Kuzminov Aug 11 '20 at 08:09
OK, I will look up sink pattern thanks. And I did try to use make_shared but I couldn't pass the newly created object into the Deleter class. Other blogs passed the raw pointer manually. I didn't think it was a red flag – Lews Therin Aug 11 '20 at 08:12

shared_ptr is getting deleted early

1 Answers1