C++ with Qt5 : lock a common mutex before calling the actual override implementation of a method

Question

I have an abstract class like this :

class Thing
{
public:
    Thing();
    virtual ~Thing();

    static QMutex s_mutex;

    virtual void load() = 0;
    virtual void translate() = 0;
    virtual void clear() = 0;
};

and various classes that are concretization of Thing. I want to be able to require the different methods to lock the s_mutex before doing anything, without having to do it manually in every implementation of them in actual classes 8which I am sure to forget to do when I implement a new concretization of Thing in some distant future). I could do this I think :

class Thing
{
public:
    Thing();
    virtual ~Thing();

    static QMutex s_mutex;

    void lockAndLoad() { QMutexLocker mutexLocker(&s_mutex); this->load(); }
    void lockAndTranslate(int amount) { QMutexLocker mutexLocker(&s_mutex); this->translate(amount); }
    void lockAndClear() { QMutexLocker mutexLocker(&s_mutex); this->clear(); }

private:
    virtual void load() = 0;
    virtual void translate(int amount) = 0;
    virtual void clear() = 0;
};

But this feels tedious and excessive/suboptimal.

I also cannot write a function that would receive another function as argument and that would lock the mutex then call the function - because my function signature/prototypes are all different (and passing a function as an argument requires to specify its prototype, if I am not mistaken).

EDIT : this is moot as I would still need to make the individual functions public and then there is risk that the user calls them directly instead of via the "lock first, then run" function I suggested above.

Can I do better ?

Answer 1

You always have the option of using the preprocessor to help declare your functions, something like:

#define DECLARE_PROTECTED_FUNCTION(fn) \
    public: void fn() { QMutexLocker mutexLocker(&s_mutex); CONCAT(fn, _internal) (); } \
    private: virtual void CONCAT(fn, _internal) () = 0;

But that's about as far as you can get using C++ code.

You then have the choice of source generation to generate this all for you based on your existing class definitions. This is much cleaner (if more arcane), but keep in mind that there isn't one general source generation API, so you'd lock yourself into specific compilers.

And lastly, consider that your approach is very coarse. Locking individual operations on individual items with a full OS mutex sounds like a performance nightmare. Surely you can write a better algorithm in 2024.

Answer 2

Once upon a time I wrote a template with very similar function.For Qt5 and modern C++ standard this would require some adjustments. Note, it was for Qt4, pre-C++11 and QMutex back then for recursive and non-recursive use was different in initiliazation. Not separate classes.

/** Mutex-protected interface.
 @param class Data - type of protected structure
 @param QMutex::RecursionMode mode - is mutex recursive?
 @example
 struct Data {
    vector<float> arr;
 };
 struct StoredData : ILockableData<Data, QMutex::Recursive >
 {
 };
*/
template <class Data, QMutex::RecursionMode mode = QMutex::NonRecursive>
class ILockableData  : public Data
{
    // de-coupling of QMutex to permit copy-assignment
    struct MutexIsolator
    {
        QScopedPointer<QMutex> lp;
        MutexIsolator(): lp(new QMutex(mode)) {}
        MutexIsolator(const MutexIsolator&): lp(new QMutex(mode)) {}
        MutexIsolator(MutexIsolator&&): lp(new QMutex(mode)) {}
        MutexIsolator& operator=(const MutexIsolator&) {}
        MutexIsolator& operator=(MutexIsolator&&) {}
    } lck;

    QMutex& mutex() const { return *lck.lp.data(); }
public:
    ILockableData() {}
    ILockableData(const ILockableData& v)  {
        this->operator=(v);
    }
    ILockableData(const ILockableData&& v)  {
        this->operator=(std::move(v));
    }

    // Mutex-like interface
    void    lock ()    { mutex().lock();}
    bool    tryLock () { return mutex().tryLock();}
    bool    tryLock ( int timeout ) { return mutex().tryLock(timeout);}
    void    unlock () { mutex().unlock();}

    /** Copy override. Data  must be copyable */
    ILockableData& operator=(const ILockableData& v) {
        if(&v == this) return *this;
        QMutexLocker l1(&(this->mutex()));
        QMutexLocker l2(&(v.mutex()));
        const Data& arg = static_cast<const Data&>(v);
        static_cast<Data*>(this)->operator=(arg);
        return *this;
    }

    /** Data must be moveable. */
    ILockableData& operator=(ILockableData&& v) {
        if(&v == this) return *this;
        QMutexLocker l1(&(this->mutex()));
        QMutexLocker l2(&(v.mutex()));
        Data&& arg = static_cast<Data&&>(v);
        static_cast<Data*>(this)->operator=(std::move(arg));
        return *this;
    }

    operator Data() {
          QMutexLocker l(&(this->lck)); 
          return  static_cast<Data&>(*this); 
    }
};

This is probably a bare imaginable minimum what is required to use CRTP. But anything derived from this class would have interfaces to operate with Data using QMutex lock.

Now if you want virtual interface for this, you have to add a Base class from which Data is derived wich would have pure definitions.

It's not effective. Protecting a single structure is in general not very effective and potentionally dangerous. Workign with massive data, we would need to protect containers, especially if they are associative.We would want to use ordering of operations.

Having a shared mutex state simplifies things. Each ILockableData<Data> would have one of their own for each Data type, while still having shared interface and we no longer have to worry about copying.

class Thing
{
public:
    Thing() {}
    virtual ~Thing() {}

protected: // you can't make this private, for progeny
    virtual void load() = 0;
    virtual void translate(int amount) = 0;
    virtual void clear() = 0;
};

template<class T>
class ThingInterface 
{
public:
    // do you want these virtual? Need a base class for that
    void lockAndLoad() { 
       QMutexLocker mutexLocker(&static_cast<T*>(this)->mutex());
       static_cast<T*>(this)->load();
    }
    void lockAndTranslate(int amount) { 
       QMutexLocker mutexLocker(&static_cast<T*>(this)->mutex()); 
       static_cast<T*>(this)->translate(amount); 
    }
    void lockAndClear() { 
       QMutexLocker mutexLocker(&static_cast<T*>(this)->mutex());  
       static_cast<T*>(this)->clear(); 
    }
};

template<class T>
class StaticLockable {
    static QMutex mut;
protected:
    static QMutex& mutex() { return mut; }
    // something else?
};

class ConcreteThing : public Thing, 
                      public StaticLockable<ConcreteThing>, 
                      public ThingInterface<ConcreteThing>
{
    // this is required to access load(); 
    // Another way is to use Accessor pattern.
    friend  ThingInterface<ConcreteThing>;

    // void lockAndLoad() and co. are public
protected:
    void load() override {}
    void translate(int amount) override {}
    void clear() override {}
};

Answer 3

You can use the same RAII scope locking technique as QMutexLocker:

struct ThingIface{
    virtual void load() = 0;
    virtual void translate(int) = 0;
    virtual void clear() = 0;
    virtual ~ThingIface() = default;
};

class Thing: ThingIface{
public:
    friend auto locked(Thing* const tng){
        class mylocker{
        public:
            mylocker(Thing * const ptr, QMutexLocker * mt):
                lock{ mt },
                that{ ptr } {};
            //make it smart ptr:
            ThingIface* operator->()const&&
            { return that; };

        private:
            //no copy, no move, no nothing:
            mylocker(mylocker&&) = delete;

            QMutexLocker const lock;
            Thing * const that;
        };//mylocker
        
        return mylocker{tng, &tng->all_mutex()};
    };//! locked(tng);
private:
    static auto& all_mutex(){
        static QMutex mx;
        return mx;
    };//all_mutex();
};//! Thing

Now derive a class ding:

class Ding: public Thing {
    void load() override;
    void translate(int) override;
    void clear() override;
};

Time to harvest all the trouble:

Ding myding;
locked(&myding)->load();
locked(&myding)->translate(0);
locked(&myding)->clear();

In this design, locker function is the gate keeper to ThingIface interface. mylocker is a RAII lock with smart pointer properties. The operator-> could be replaced by a get accessor, but that would create a potential risk for storing the pointer and use after unlock. To minimize the error margine, I narrowed the life time of mylocker to prvalue-only. That's why the operator-> is restricted to r-value(&& qualifier). This way, the result of locked function is forced to be immediately used as an argument to ThingIface methods.