Safe destruction of a map containing shared_ptr

Question

I was reading the Mir project source code and I stumbled upon this piece of code :

void mir::frontend::ResourceCache::free_resource(google::protobuf::Message* key)
{
    std::shared_ptr<void> value;
    {
        std::lock_guard<std::mutex> lock(guard);

        auto const& p = resources.find(key);

        if (p != resources.end())
        {
            value = p->second;
        }

        resources.erase(key);
    }
}

I have seen this before in other projects as well. It holds a reference to the value in the map before its erasure, even when the bloc is protected by a lock_guard. I'm not sure why they hold a reference to the value by using std::shared_ptr value.

What are the repercussions if we remove the value = p->second ?

Will someone please enlighten me ?

This is the code http://bazaar.launchpad.net/~mir-team/mir/trunk/view/head:/src/frontend/resource_cache.cpp

I'm confused about which bit you're asking about. Are you asking why it is a reference and not copied? — Joseph Mansfield, Mar 11 '13 at 16:53
Maybe the question is if/why assigning p->second to value is a way of freeing it. — Sarien, Mar 11 '13 at 16:57
Can you provide a link to the real code in the project? I don't think the code as-is makes much sense for multiple reasons... — David Rodríguez - dribeas, Mar 11 '13 at 17:08
@DavidRodríguez-dribeas http://bazaar.launchpad.net/~mir-team/mir/trunk/view/head:/src/frontend/resource_cache.cpp — Ghassen Hamrouni, Mar 11 '13 at 17:10

score 3 · Accepted Answer · answered Mar 11 '13 at 16:59

3

My guess it that this is done to avoid running the destructor of value inside the locked code. This lock is meant to protect the modification of the map, and running some arbitrary code, such as the destructor of another object with it locked, is not needed nor wanted.

Just imagine that the destructor of value accesses indirectly, for whatever reason to the map, or to another thread-shared structure. There are chances that you end up in a deadlock.

The bottom end is: run as little code as possible from locked code, but not less. And never call a external, unknown, function (such as the shared_ptr deleter or a callback) from locked code.

answered Mar 11 '13 at 16:59

rodrigo

94,151
12
143
190

1

+1, this is most probably the reason. Other than that `auto const& p` should be `auto p`, and the erase should be `resources.erase(p)` inside the `if` condition... – David Rodríguez - dribeas Mar 11 '13 at 17:17
@DavidRodríguez-dribeas: Agreed with the latter, but why the `auto p`? Because you know it is an iterator, and iterators are cheap-copyable? Or is there a deeper reason? Wouldn't it be better `auto &&p`? – rodrigo Mar 11 '13 at 17:27
`auto&&` would definitely not be better. The `auto p` is because the function returns by *value*, which means that the copy will be done regardless of whether `p` is a reference or value, but the fact that it shows as a reference appears as if you are getting a reference to some object inside `resources` which is not the case. I don't like to abuse binding of a `const&` to a temporary to extend the lifetime because it hides what happens from the reader. Why `auto&&p` is worse? Because you would bind a reference to the temporary, the temporary goes out of scope and you get a dangling reference. – David Rodríguez - dribeas Mar 11 '13 at 17:39
@DavidRodríguez-dribeas: About the `auto&&` I was thinking more about [Universal References](http://isocpp.org/blog/2012/11/universal-references-in-c11-scott-meyers). And I disagree about the dangling reference: I tested the code and the lifetime of the temporary is extended to the scope of the r-reference, just as with an l-reference. – rodrigo Mar 11 '13 at 20:47
@rodrigo If the function returns by value its lifetime is extended. If it returns by rvalue reference, there could be dangling reference. http://stackoverflow.com/a/14849480/463758 – balki Mar 12 '13 at 07:12

score 2 · Answer 2 · answered Mar 11 '13 at 17:00

2

The goal is to move the actual execution of the shared_ptr deleter till after the lock is released. This way, if the deleter (or the destructor using the default deleter) takes a long time, the lock is not held for that operation.

If you were to remove the value = p->second, the value would be destroyed while the lock is held. Since the lock protects the map, but not the actual values, it would hold the lock longer than is strictly necessary.

answered Mar 11 '13 at 17:00

Dave S

20,507
3
48
68

3

Note also that the deleter will often involve a free store operation which is likely to be independently synchronized (eg. via a mutex around the heap). This not only means it's likely to be relatively slow, but also means it could be blocking, and greatly increases the chance of contention. – Useless Mar 11 '13 at 17:05

Safe destruction of a map containing shared_ptr

2 Answers2