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.