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 <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(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 {
return &m_data.get();
}

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

#ifndef NDEBUG
#endif
};

// Starts multiple transactions simultaneously in a
template<typename... Args>
auto transaction(Args&&... args) {
std::lock(args...);
}

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(); }

}

}

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

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

int main() {

{
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.

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
template<typename... Args>
auto transaction(Args&&... args) {
std::lock(args...);
}


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...);
}


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...);
}
};


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()...);
}
};


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.

• 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. – Emily L. Aug 10 '15 at 17:42

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 <mutex>
#include <cassert>
#include <memory>
#include <iostream>

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

class simple_tester
{

public:

{
std::unique_lock<std::mutex> alock(m_data_lock);
m_sample_transaction.func1();
}

{
std::unique_lock<std::mutex> alock(m_data_lock);
m_sample_transaction.func2();

}

void run_test()
{

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

}

private:
std::mutex          m_data_lock;

• 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... – Emily L. Jul 9 '15 at 0:23