11
\$\begingroup\$

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.

\$\endgroup\$
2
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Thanks for the review. The design of the class does full-fill its purpose, prevents accidental non-exclusive access to the protected data in legacy code. \$\endgroup\$ – Emily L. Aug 10 '15 at 17:42
-2
\$\begingroup\$

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();
}
\$\endgroup\$
  • \$\begingroup\$ The problem being solved is that I need to enforce transaction semantics on legacy classes. There are no "transaction objects floating about". The comment about the thread ID was more of a prevent stupid mistakes in debug mode type of deal. The main point of this is to make sure that the user doesn't manipulate the datum without a mutex for the entire duration of the transaction. \$\endgroup\$ – Emily L. Jul 8 '15 at 16:04
  • \$\begingroup\$ While I like the threadID debugging technique , I simply do not understand how you are going to enforce it, moreover how you going to use same instance of lock in different threads. Your code is incredibly complex for what it is doing ( I would hate to be a maintenance programmer working with it ) , yet it seems to miss the very functionality it advertises \$\endgroup\$ – AAO Jul 8 '15 at 17:02
  • \$\begingroup\$ Quite frankly you don't make much sense to me. How am I going to enforce what exactly? I'm not using the same unique_lock from different threads that's the whole point of the assert on the thread Id, to make sure the user doesn't do that. If you meant the mutex then that's the way you're supposed to use a mutex... The code is not at all complex, quite the opposite it aims to reduce complexity of explicitly dealing with locking and automating the process in a foolproof way. You have the wrong idea of what the code accomplishes, but I can't figure out what it is you don't understand... \$\endgroup\$ – Emily L. Jul 9 '15 at 0:23

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.