Thread safety/Transaction enforcer

Question

I have some legacy classes written without thread safety in mind. Instances of these classes are now being accessed in a multithreaded context in a thread-un-safe manner. Cue chaos.

To fix this I make a wrapper class that enforces the user to take a transaction lock to serialize access to the instance.

#include <memory>
#include <thread>
#include <mutex>
#include <cassert>

namespace trapi {
    template<typename T>
    class Transaction {
    public:
        Transaction(T& a_data, std::mutex& a_mutex)
            : m_data(a_data), m_lg(a_mutex)
        {}

        Transaction(T& a_data, std::mutex& a_mutex, std::adopt_lock_t)
            : m_data(a_data), m_lg(a_mutex, std::adopt_lock)
        {}

        Transaction(const Transaction&) = delete;

        Transaction(Transaction&& a_that)
            : m_data(a_that.m_data), m_lg(std::move(a_that.m_lg))
        {}

        ~Transaction() = default;

        Transaction& operator = (const Transaction& a_that) = delete;
        Transaction& operator = (Transaction&& a_that) = delete;

        T* operator -> () const {
            assert(m_owner == std::this_thread::get_id());
            return &m_data.get();
        }

    private:
        std::reference_wrapper<T> m_data;
        std::unique_lock<std::mutex> m_lg;

#ifndef NDEBUG
        std::thread::id m_owner = std::this_thread::get_id();
#endif
    };

    // Starts multiple transactions simultaneously in a 
    // dead-lock free manner.
    template<typename... Args>
    auto transaction(Args&&... args) {
        std::lock(args...);
        return std::make_tuple(args.adopt_transaction()...);
    }

    template<typename T>
    class TransactionEnforcer {
    public:
        template<typename... Args>
        TransactionEnforcer(Args&&... args)
            : m_data(std::forward<Args>(args)...)
        {}

        TransactionEnforcer(const TransactionEnforcer&) = delete;
        TransactionEnforcer(TransactionEnforcer&&) = delete;
        ~TransactionEnforcer() = default;
        TransactionEnforcer& operator = (const TransactionEnforcer&) = delete;
        TransactionEnforcer& operator = (TransactionEnforcer&&) = delete;

        Transaction<T> transaction() {
            return Transaction<T>(m_data, m_mutex);
        }

        Transaction<const T> transaction() const {
            return Transaction<T>(m_data, m_mutex);
        }

        void lock() { m_mutex.lock(); }
        bool try_lock() { return m_mutex.try_lock(); }
        void unlock() { m_mutex.unlock(); }

        Transaction<T> adopt_transaction() {
            return Transaction<T>(m_data, m_mutex, std::adopt_lock);
        }

        Transaction<const T> adopt_transaction() const {
            return Transaction<T>(m_data, m_mutex, std::adopt_lock);
        }

    private:
        T m_data;
        std::mutex m_mutex;
    };
}

class ThreadUnsafeClass {
public:
    void func1() {}
    void func2() {}
};

using ThreadSafeClass = trapi::TransactionEnforcer<ThreadUnsafeClass>;

int main() {
    ThreadSafeClass data1;
    ThreadSafeClass data2;

    {
        auto t = data1.transaction();
        t->func1();
        t->func2();
    }

    {
        auto ct = trapi::transaction(data1, data2);
        auto& t1 = std::get<0>(ct);
        auto& t2 = std::get<1>(ct);
        t1->func1();
        t2->func2();
    }
}

In general I'm happy with the design and it will solve the problem elegantly IMHO.

But what I'm concerned about is the debug m_owner in the Transaction class. I want something to alert me when some one has managed to share a Transaction object between threads. But I'm not sure if this is actually a problem if it happens. Am I being to strict here?

Also the free function transaction that allows one to start multiple transactions without ending up in a deadlock. To be able to implement it was forced to make lock(), try_lock(), unlock() and adopt_transaction() public which I'm not happy about as I don't want the user fiddling with these and possibly breaking something by forgetting to unlock and what have you. I tried to declare the transaction function as a template friend but I couldn't figure out the return type. So I would like some help with that.

I'm also very keen on ideas for better names. I'm not 100% sold on the current names but I can't come up with anything better.

Answer 1

Also the free function transaction that allows one to start multiple transactions without ending up in a deadlock. To be able to implement it was forced to make lock(), try_lock(), unlock() and adopt_transaction() public which I'm not happy about as I don't want the user fiddling with these and possibly breaking something by forgetting to unlock and what have you. I tried to declare the transaction function as a template friend but I couldn't figure out the return type. So I would like some help with that.

// Starts multiple transactions simultaneously in a 
// dead-lock free manner.
template<typename... Args>
auto transaction(Args&&... args) {
    std::lock(args...);
    return std::make_tuple(args.adopt_transaction()...);
}

First of all, I don't think this is a good use of variadic templates. IIUC, you're trying to make a function that accepts any number of TransactionEnforcer<T> objects and returns a std::tuple<Transaction<T>...>? I would express that at least as

template<typename... T>
auto transaction(TransactionEnforcer<T>&&... args) {
    std::lock(args...);
    return std::make_tuple(args.adopt_transaction()...);
}

and possibly throw in the -> std::tuple<Transaction<T>...> since it's pretty easy to express; but bearing in mind that when you provide an explicit return type you enable SFINAE on that return type, which could theoretically turn some wrong code from a compiler error ("call to transaction is ambiguous...") into code that happily does the wrong thing (since one of the candidates is now SFINAE'd away). That's highly theoretical, though.

Now, how do you make this function a friend of TransactionEnforcer? That's easy; just define it inline!

template<class T>
class TransactionEnforcer
{
    // ...

    template<typename... T>
    friend auto transaction(TransactionEnforcer<T>&&... args) {
        std::lock(args...);
        return std::make_tuple(args.adopt_transaction()...);
    }
};

This would allow you to make adopt_transaction a private member function; but it would not allow you to make lock, try_lock, and unlock private member functions, because they're still being touched by someone who's not a friend of the class; namely, they're being touched by std::lock().

To fix this, you could expose a private inner class LockView:

template<class T>
class TransactionEnforcer
{
    // ...
  private:
    void lock();
    bool try_lock();
    void unlock();
    friend struct LockView {
        TransactionEnforcer& te_;
        void lock() { return te_.lock(); }
        bool try_lock() { return te_.try_lock(); }
        void unlock() { return te_.unlock(); }
    };
    LockView lock_view() { return LockView{*this}; }

    template<typename... T>
    friend auto transaction(TransactionEnforcer<T>&&... args) {
        std::lock(args.lock_view()...);
        return std::make_tuple(args.adopt_transaction()...);
    }
};

Unfortunately, this doesn't work out of the box because std::lock() requires that its arguments all be lvalues. So you'll have to make up some storage for the LockView objects somewhere: either use parameter-pack expansion to turn your parameter-pack full of TransactionEnforcers into a tuple full of LockViews suitable for passing (via std::apply?) to std::lock, or else make te.lock_view() return an lvalue reference to te.lv_, which is a self-referential LockView subobject of te. Fun times. But you can make it work.

No comment on the basic functionality of TransactionEnforcer; I'm not sure it's doing what you really want done, but maybe it is.

Answer 2

I might be missing something, but from the looks of it, you are solving non-existent problem at a cost of the performance (big time). From your description it sounds like you have huge multi-threaded system where transaction objects are floating around and they are being picked up at random to be executed. If that is the case, I would concentrate on the executing part and address the thread safety there. Making a transaction object, especially small one, be "thread-safe" seems strange.

Added the simple tester, how would it look using your contraption? Will it work?

#include <memory>
#include <thread>
#include <mutex>
#include <cassert>
#include <memory>
#include <iostream>

class  ThreadUnsafeClass 
{
    public:
        void func1() { std::cout << "func1" << std::endl; }         
        void func2() { std::cout << "func2" << std::endl; }
};


class simple_tester
{

public:


    void sample_trhread_proc1()
    {
        std::unique_lock<std::mutex> alock(m_data_lock);
        std::this_thread::sleep_for(std::chrono::seconds(4));
        m_sample_transaction.func1();
    }



    void sample_trhread_proc2()
    {
        std::this_thread::sleep_for(std::chrono::seconds(2));
        std::unique_lock<std::mutex> alock(m_data_lock);
        m_sample_transaction.func2();



    }


    void run_test()
    {
        std::thread th1(&simple_tester::sample_trhread_proc1, this);
        std::thread th2(&simple_tester::sample_trhread_proc2, this);

        if (th1.joinable())
            th1.join();
        if (th2.joinable())
            th2.join();

    }



private:
    std::mutex          m_data_lock;
    ThreadUnsafeClass   m_sample_transaction;
};


int main()
{
    simple_tester().run_test();
}