4

Most of the times I see in the code some variant of this kind of implementation for a thread safe getter method:

class A
{
public:

    inline Resource getResource() const
    {
        Lock lock(m_mutex);

        return m_resource;
    }

private:
    Resource m_resource;
    Mutex    m_mutex;
};

Assuming that the class Resource can't be copied, or that the copy operation has a too high computational cost, is there a way in C++ to avoid the returning copy but still using a RAII style locking mechanism?

nyarlathotep108
  • 5,275
  • 2
  • 26
  • 64
  • Which C++ version are you using? – Simon Kraemer Nov 12 '15 at 15:24
  • 1
    "is there a way to avoid returning copy" yes do not return it at all. – Slava Nov 12 '15 at 15:25
  • Return `const Resource&`? – cadaniluk Nov 12 '15 at 15:25
  • 1
    @cad That'd not be thread-safe – unexpectedvalue Nov 12 '15 at 15:26
  • 1
    @cad the problem with returning a reference is that the lock is destroyed after the getResource call via RAII, and whoever was given that reference now has access to m_resource without the resource being locked. – YoungJohn Nov 12 '15 at 15:27
  • @SimonKraemer C++11, but if you can provide an answer which also shows the differences between the various versions it would be great – nyarlathotep108 Nov 12 '15 at 15:27
  • @YoungJohn Oops, somehow didn't notice that. ^^ – cadaniluk Nov 12 '15 at 15:28
  • It would be nice if you had a way to break up the Resource into smaller copyable pieces or provide accessors to specific parts of Resource (this way one thread can view or use some part of Resource while another can still work on a different piece, but that is not often feasible). It might be nice to have a helper function that allows you to get a smaller-copyable snapshot or proxy of Resource. – YoungJohn Nov 12 '15 at 15:39

3 Answers3

4

How about returning an accessor object that provides a thread-safe interface to the Resource class and/or keeps some lock?

class ResourceGuard {
private:
    Resource *resource;

public:
    void thread_safe_method() {
        resource->lock_and_do_stuff();
    }
}

This will be cleared in a RAII fashion, releasing any locks if needed. If you need locking it should be done in the the Resource class.

Of course you have to take care of the lifespan of Resource. A very simple way would be to use a std::shard_ptr. A weak_ptr might fit as well.

unexpectedvalue
  • 6,079
  • 3
  • 38
  • 62
  • 1
    Beat me to it. Personally, I would store a reference to `m_resource` rather than a pointer and return the guard by `std::unique_ptr`. A `std::shard_ptr` could be copied to other threads and many threads owning the same mutex would be a disaster. – eerorika Nov 12 '15 at 15:33
  • Could you please integrate the code in your answer with the getter code? I can't see how can this work if I return every time a ResourceGuard object and it has its own mutex inside... – nyarlathotep108 Nov 12 '15 at 15:34
  • You are right of course. The locking should be done when accessing interfaces. – unexpectedvalue Nov 12 '15 at 15:39
4

another way to achieve the same thing. This is the implementation of a mutable version. the const accessor is just as trivial.

#include <iostream>
#include <mutex>

struct Resource
{

};

struct locked_resource_view
{
    locked_resource_view(std::unique_lock<std::mutex> lck, Resource& r)
    : _lock(std::move(lck))
    , _resource(r)
    {}

    void unlock() {
        _lock.unlock();
    }

    Resource& get() {
        return _resource;
    }

private:
    std::unique_lock<std::mutex> _lock;
    Resource& _resource;
};

class A
{
public:

    inline locked_resource_view getResource()
    {
        return {
            std::unique_lock<std::mutex>(m_mutex),
            m_resource
        };
    }

private:
    Resource m_resource;
    mutable std::mutex    m_mutex;
};

using namespace std;

auto main() -> int
{
    A a;
    auto r = a.getResource();
    // do something with r.get()

    return 0;
}
Richard Hodges
  • 68,278
  • 7
  • 90
  • 142
  • Now this is even closer to what I was about to post (than ssteinberg's original answer). I would define the constructor like this: `locked_resource_view(std::mutex& m, Resource& r)`. That way the lock can be constructed in-place. – eerorika Nov 12 '15 at 16:01
  • Sure that's probably neater. In any case the redundant move will most likely be optimised away. – Richard Hodges Nov 12 '15 at 16:03
4

I haven't tried it, but something like this should work:

#include <iostream>
#include <mutex>
using namespace std;

typedef std::mutex Mutex;
typedef std::unique_lock<Mutex> Lock;

struct Resource {
    void doSomething() {printf("Resource::doSomething()\n"); }
};

template<typename MutexType, typename ResourceType>
class LockedResource
{
public:
    LockedResource(MutexType& mutex, ResourceType& resource) : m_mutexLocker(mutex), m_pResource(&resource) {}
    LockedResource(MutexType& mutex, ResourceType* resource) : m_mutexLocker(mutex), m_pResource(resource) {}
    LockedResource(LockedResource&&) = default;
    LockedResource(const LockedResource&) = delete;
    LockedResource& operator=(const LockedResource&) = delete;

    ResourceType* operator->()
    {
        return m_pResource;
    }

private:
    Lock m_mutexLocker;
    ResourceType* m_pResource;
};

class A
{
public:

    inline LockedResource<Mutex, Resource> getResource()
    {
        return LockedResource<Mutex, Resource>(m_mutex, &m_resource);
    }

private:
    Resource m_resource;
    Mutex    m_mutex;
};


int main()
{
    A a;
    { //Lock scope for multiple calls
        auto r = a.getResource();
        r->doSomething();
        r->doSomething();

        // The next line will block forever as the lock is still in use
        //auto dead = a.getResource();

    } // r will be destroyed here and unlock
    a.getResource()->doSomething();
    return 0;
}

But be careful, as the lifetime of the accessed Resource depends on the lifetime of the owner (A)


Example on Godbolt: Link

P1144 reduces the generated assembly quite nicely so that you can see where the lock is locked and unlocked.

Simon Kraemer
  • 5,700
  • 1
  • 19
  • 49
  • can't understand the reason for the nested scope in main() – nyarlathotep108 Nov 12 '15 at 16:24
  • 1
    @nyarlathotep108 That scope was meant to indicate where the lock starts and ends, but I messed it up by not storing the instance of the LockedRessource. Just ignore it. I'm currently on my mobile but I'll remove that scope tomorrow. – Simon Kraemer Nov 12 '15 at 20:05
  • @nyarlathotep108 I hope it makes more sense now. – Simon Kraemer Nov 13 '15 at 06:04
  • Turns out I prematurely upvoted this one. It doesn't compile unfortunately. The first problem is with `LockedResource`s move constructor, which is invalidated by using `std::lock_guard` here, which is not movable. Replacing that by an `std::unique_lock` fixes this. The next problem is with `A::getResource()`'s return type, which essentially leads to returning a reference to a local temporary. Replacing that simply by `LockedResource` fixes this. .Could you please edit your answer to make it valid? Thanks! – ahans Dec 02 '19 at 09:54
  • 1
    @ahans Like I mentioned I hadn't tested the code and it was meant more like some kind of template. Thank you for pointing out the flaws - I fixed the mentioned points now. – Simon Kraemer Jan 07 '20 at 12:24