std::move return value of weak_ptr::lock messing up reference count of shared_ptr?

Question

i need an explanation of the following behaviour:

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

struct A {
    std::string s = "foo";
    std::weak_ptr<A> h;
    std::shared_ptr<A> && getR() {
        return std::move(h.lock());
    }
    std::shared_ptr<A> getL() {
        return h.lock();
    }
};

std::vector< std::shared_ptr<A> > storage;
std::vector< std::weak_ptr<A> > accountant;

void store(std::shared_ptr<A> && rr) {
    std::cout << "store '" << rr->s << "' uses: " << rr.use_count() << std::endl;
    storage.push_back(std::move(rr));
}

int main() {
    // create keeper of A
    auto keeper = std::make_shared<A>();
    keeper->s = "bar";
    // store weak_ptr-type handle with accountant
    accountant.push_back(keeper);
    // backref handle to A
    keeper->h = accountant[0];

    std::cout << "# case 0: manual 'move'" << std::endl;
    {
        store(std::move(accountant[0].lock()));

        std::cout << "uses: " << keeper.use_count() << std::endl;
    }
    storage.clear();

    std::cout << "# case 1: manual 'move' from internal" << std::endl;
    {
        store(std::move(keeper->h.lock()));

        std::cout << "uses: " << keeper.use_count() << std::endl;
    }
    storage.clear();

    std::cout << "# case 2: return copy from func" << std::endl;
    {
        store(keeper->getL());

        std::cout << "uses: " << keeper.use_count() << std::endl;
    }
    storage.clear();
    // all is well up to here.

    std::cout << "# case 3: return rref from func" << std::endl;
    {
        store(keeper->getR());

        std::cout << "uses: " << keeper.use_count() << std::endl;
        std::cout << "storage[0]: " << storage[0].get() << " uses: " << storage[0].use_count() << " " << &storage[0] << std::endl;
        std::cout << "keeper: " << keeper.get() << " uses: " << keeper.use_count() << " " << &keeper << std::endl;
    }
    storage.clear();

    std::cout << "# after" << std::endl;
    std::cout << "uses: " << keeper.use_count() << std::endl;
    // all the A is gone!!!!
    return 0;
}

output:

# case 0: manual 'move'
store 'bar' uses: 2
uses: 2
# case 1: manual 'move' from internal
store 'bar' uses: 2
uses: 2
# case 2: return copy from func
store 'bar' uses: 2
uses: 2
# case 3: return rref from func
store 'bar' uses: 1
uses: 1
storage[0]: 0x2b49f7a0fc30 uses: 1 0x2b49f7a10ca0
keeper: 0x2b49f7a0fc30 uses: 1 0x7ffd3683be20
# after
uses: 0

ideone: http://ideone.com/smt7TX

This is a class holding a weak_ptr to itself, so it can give out shared_ptr-handles to itself. Its a resource-class in the real code, and shared_ptr handles to those get passed around. Now in an effort to reduce copying shared_ptrs i came across my getHandle function (getR/getL in the above) and wanted it to return by moving instead of copying. In a short test std::moving the return of weak_ptr::lock seemed ok, but in the final code it messed things up bad. In comparison to copying the return-value it seems moving it reduces the shared_ptr's reference counter - so i end up with 2 shared_ptrs in existence but both having a use_count() of 1. so if the one i got using get() goes out of scope the A gets destroyed and my original shared_ptr which is still around points to garbage. In the example code you can see that after case 3 - i would have expected the last cout to tell me a use_count() of 1 until keeper is destroyed.

Now in the real code i just inlined the equivalent of getL in the hopes that this will prevent the superflous copying, but i can't get over not having a clue why this doesn't work as i thought it would.

Why does case 3 reduce the reference count? And then why don't case 0 and 1 also reduce it?

using std::move in a return is normally a pessimization. If you want a prvalue then just return by value from the function. — NathanOliver
– NathanOliver, Commented May 4, 2017 at 13:25
Yes, why are you moving temporaries? You can't make them any more rvaluey than they already are. — Jonathan Wakely
– Jonathan Wakely, Commented May 4, 2017 at 13:30
"This is a class holding a weak_ptr to itself, so it can give out shared_ptr-handles to itself. " Why not derive from std::enable_shared_from_this<A> and have it work automatically with no extra effort and no reference cycles? — Jonathan Wakely
– Jonathan Wakely, Commented May 4, 2017 at 13:31
Jonathan and Nathan, yes... why indeed ... i should have known that. the getHandle function started out bigger and boiled down more and more - as it reached it's final short form i must have been blinded. Also thanks for the hint to std::enable_shared_from_this that's just what i need! — Jonas Beck
– Jonas Beck, Commented May 4, 2017 at 13:38
The weird reference counts are because you have undefined behaviour, because you return a dangling reference thanks to that std::move. It turns an rvalue into a reference to empty space. — Jonathan Wakely
– Jonathan Wakely, Commented May 4, 2017 at 13:42

Community · Accepted Answer · 2017-05-23 10:31:25Z

8

You have a bug here:

std::shared_ptr<A> && getR() {
    return std::move(h.lock());
}

This creates a temporary shared_ptr which is local to the function then returns a reference to it. That is a dangling reference to an object that no longer exists. Just return by value as getL does (I don't know why you've called it getL ... if that refers to an lvalue it's wrong, it returns an rvalue).

You are misusing std::move in a misguided attempt to improve performance, but simply returning the object is simpler, safer, and allows the compiler to optimise it far more effectively. Without the std::move there won't be any copy or move, the compiler will elide the temporary completely, see What are copy elision and return value optimization?

These other moves are also redundant (although not actually harmful here):

    store(std::move(accountant[0].lock()));

    store(std::move(keeper->h.lock()));

In both cases you're trying to move something that is already an rvalue, which is pointless.

Also you have reimplemented std::enable_shared_from_this, poorly. Get rid of your weak_ptr member and your backref and just do:

struct A : std::enable_shared_from_this<A> {
    std::string s = "foo";
};

And then call keeper->shared_from_this() instead of keeper->getL(). You'll notice that shared_from_this() returns by value, not by reference, to avoid the bug in your getR() function.

edited May 23, 2017 at 10:31

CommunityBot

11 silver badge

answered May 4, 2017 at 13:36

Jonathan Wakely

172k28 gold badges360 silver badges540 bronze badges

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

1 Comment

Jonas Beck Over a year ago

Thanks for taking the time.

Collectives™ on Stack Overflow

std::move return value of weak_ptr::lock messing up reference count of shared_ptr?

1 Answer 1

1 Comment

Your Answer

Linked

Hot Network Questions

Collectives™ on Stack Overflow

1 Answer 1

1 Comment

Your Answer

Sign up or log in

Post as a guest

Linked

Related