C++ lock wrapper class around pointer

Question

I have created a class which wraps pointers and behaves like a pointer would behave, minus a few cases. It is a lock wrapper class which simply locks before usage of the pointer and unlocks once the pointer usage is done.

It must behave exactly like a pointer EXCEPT in the following cases:

It cannot be implicitly converted to the pointer type it is pointing to
It does not handle any implicit memory allocation/deallocation (it provides a method to delete the pointer to the object)
When compared to another wrapper object, it compares their pointers to see if they are the same

Here is what I have so far:

template <class PtrType, class LockType>
class ELockWrapper {
public:
    class Proxy {
    public:
        Proxy(PtrType* p, LockType* lock) : ptr(p), mLock(lock)  { mLock->Lock(); }
        ~Proxy()                                                 { mLock->Unlock(); }
        PtrType* operator->()                                    { return ptr; }
        PtrType  operator*()                                     { return *ptr; }
    private:
        PtrType*    ptr;
        LockType*   mLock;
    };

    ELockWrapper()                            : ptr(nullptr), lock(nullptr) {}
    ELockWrapper(nullptr_t t)                 : ELockWrapper()   {}
    ELockWrapper(PtrType *p, LockType* l)     : ptr(p), lock(l)  {}
    ELockWrapper(PtrType *p, LockType& l)     : ptr(p), lock(&l) {}
    ELockWrapper(const ELockWrapper& copy) = default;

    ELockWrapper& operator=(const ELockWrapper& x) = default;
    bool operator==(const ELockWrapper& cmp) { return cmp.ptr == ptr; }
    bool operator!=(const ELockWrapper& cmp) { return !operator==(cmp); }
    bool operator==(PtrType* t)              { return ptr == t; }
    bool operator!=(PtrType* t)              { return ptr != t; }
    bool operator==(bool b)                  { return (ptr && b) || (!ptr && !b); }
    bool operator!=(bool b)                  { return !operator==(b); }
    operator bool() const                    { return ptr; }

    Proxy operator->() {
        return Proxy(ptr, lock);
    }

    PtrType operator*() {
        return *Proxy(ptr, lock);
    }

    void Delete() {
        Proxy(ptr, lock);
        delete ptr;
    }

private:
    PtrType*    ptr;
    LockType*   lock;
};

Here is it in action with some test cases.

Any mistakes/suggestions would be much appreciated.

One quick thing I want to ask: if ANY of the methods on ELockWrapper can be called concurrently, should I wrap each overloaded boolean operator with a lock? I'm thinking perhaps the delete method will be called, which a thread was interrupted in one of the operators might be problematic. Just a confirmation if this is the right thing to do?

Your Delete() method is probably wrong (can't tell as there is no specification). The current behavior is LOCK, UNLOCK, delete ptr. — Ben Voigt
– Ben Voigt, Commented Sep 27, 2016 at 20:04
Your operator-> returns an object with a lock count of one (likely expected) but it does so by locking twice and unlocking once. You may want to implement move semantics. — Ben Voigt
– Ben Voigt, Commented Sep 27, 2016 at 20:05
The name PtrType is misleading, as that type is not a pointer. The standard uses value_type for the target of a pointer or reference. — Ben Voigt
– Ben Voigt, Commented Sep 27, 2016 at 20:07
Lock() and Unlock() are incompatible function names (due to the capital letter) with the standard C++ library classes. If you simply named them lock() and unlock(), you wouldn't require a wrapper struct just to do so. — user2296177
– user2296177, Commented Sep 27, 2016 at 20:20

score 3 · Accepted Answer · 2016-09-27 22:14:31Z

operator== and operator!= should be const.
You need to #include <cstddef> to use nullptr_t.
You are defaulting the copy constructor and assignment operator, but not the move constructor and move assigment operator. That's fine because the defaults are generated anyway, but it is inconsistent, I think.
What is the point of operator==(bool)?
Proxy should be private. You don't want to have users create Proxys.
ELockWrapper does not actually own the pointer it holds. It is copyable and will just copy the pointer it holds. It also does not create anything or delete anything in the constructor/destructor. Nontheless you provide a Delete member. That seems very wrong to me. You don't know how the memory you point to was aquired, it could be with static storage or an array. In both cases calling Delete would cause undefined behavior. The object creating the pointer with new should also be responsible for deleteing it. Calling Delete on two copies of ELockWrapper also causes undefined behavior.
The fact that the pointer is not owned seems to be a major flaw here anyway. Currently the user of your class needs to gurantee that the object pointed to keeps in scope until all ELockWrapper's holding it are destroyed and then it has to take care of proper deletion. Using it outside of ELockWrapper is not allowed. So why not move the responsibility of managing the pointer to ELockWrapper?
The same holds true for the lock. Currently the user needs to provide a suitable lock and keep it live until no ELockWrapper references it anymore. Why not create the lock/mutex inside the constructor from pointer? Though I realize if you do it like this you need to manage references to the lock and you end up with fully reimplementing std::shared_ptr. Therefore I will just assume that this wrapper is explicitly only about locking and not managing ressource lifetime and that the user is required to run the destructors properly.
operator-> is fine if copy elision is performed (not required in C++11, but in C++17), but operator* is not either way. The Proxy object you create in operator* is destroyed before the function returns. That means that Unlock is called before the caller expression using the pointer ends.
operator* currently returns a copy. It should return a reference. At least that is the usual way this operator is interpreted.
In general the approach with operator* and operator-> has some limitations. You may not call either twice in one expression or you have deadlock. But you are also not allowed to save the pointer returned by operator* or a pointer/reference to the object referred because these are not guarded any more by the lock.

I guess it would be much better to let the caller call a aquire which returns a Proxy. Access to the holding pointer is then only allowed via operator* and operator-> of this Proxy. As soon as Proxy goes out of scope its destructor releases the lock, similar to std::lock_guard.

This still allows the user to misuse a saved reference/pointer to the raw object, but at least more than one usage can me made in the same lock aquisition.

An implementation via C++11 standard library would be similar to this (not tested):

template<typename T, typename Mutex>
class ELockWrapper {
    T* t;
    Mutex* mut;

    class Proxy {
        T* t;
        std::unqiue_lock<Mutex> lock;
        Proxy(ELockWrapper& wr) : t(wr->t), lock(wr->mut) { }
        friend ELockWrapper;
    public:
        T& operator*() { return *t; }
        T* operator->() { return t; }
    };

public:
    ELockWrapper() : t(nullptr), mut(nullptr) {}
    ELockWrapper(nullptr_t) : ELockWrapper() {}
    ELockWrapper(T* t, Mutex* mut) : t(t), mut(mut) {}
    ELockWrapper(T* t, Mutex& mut) : t(t), mut(&mut) {}

    Proxy aquire() { return {*this}; }
};

I did not know that C++17 requires copy elision on return. Do you have a reference? — Loki Astari
– Loki Astari, Commented Sep 28, 2016 at 2:04
I would write something very similar so I am not going to. I would just add one point on Delete(). The object is deleted but the pointer is not reset. So its hard to tell that the pointer is deleted so you can still attempt to de-reference or even delete again. — Loki Astari
– Loki Astari, Commented Sep 28, 2016 at 2:05
@LokiAstari I got that from cppreference.com. I think the correct standard reference would be P0135r1 which was adopted according to this. It only applies to specific cases which are fullfilled here for operator-> I think. — user116966
– user116966, Commented Sep 28, 2016 at 6:11

Stack Exchange Network

C++ lock wrapper class around pointer

1 Answer 1

You must log in to answer this question.

Hot Network Questions

C++ lock wrapper class around pointer

1 Answer 1

You must log in to answer this question.

Related

Hot Network Questions