Synchronization of transaction processing

Question

There's a server which does the following:

1. Receive request with transaction id
2. Load corresponding transaction from storage. New transaction object is returned each time
3. Process transaction
4. Save updated transaction back to storage

The task is to properly synchronize steps 2, 3 and 4.

Since for each request new instance of transaction object will be created, I decided to create TransactionService class with AcquireTransactionLock method to be called by request handling code.

class Transaction
{
    public int Id { get; set; }
    public int Counter { get; set; }

    public void Process()
    {
        Counter++;
    }
}

class TransactionService
{
    private readonly object syncRoot;
    private readonly Dictionary<int, Tuple<int, object>> transactionLockMap; // id -> (referenceCount, lock).

    public TransactionService()
    {
        syncRoot = new object();
        transactionLockMap = new Dictionary<int, Tuple<int, object>>();
    }

    public IDisposable AcquireTransactionLock(int transactionId)
    {
        return new TransactionLock(transactionId, this);
    }

    class TransactionLock : IDisposable
    {
        private readonly int transactionId;
        private readonly TransactionService transactionService;

        public TransactionLock(int transactionId, TransactionService transactionService)
        {
            this.transactionId = transactionId;
            this.transactionService = transactionService;

            Tuple<int, object> transactionLock;

            lock (transactionService.syncRoot)
            {
                if (!transactionService.transactionLockMap.TryGetValue(transactionId, out transactionLock))
                {
                    transactionLock = Tuple.Create(1, new object());
                }
                else
                {
                    transactionLock = Tuple.Create(transactionLock.Item1 + 1, transactionLock.Item2);
                }

                transactionService.transactionLockMap[transactionId] = transactionLock;
            }

            Monitor.Enter(transactionLock.Item2);
        }

        public void Dispose()
        {
            Tuple<int, object> transactionLock;

            lock (transactionService.syncRoot)
            {
                transactionLock = transactionService.transactionLockMap[transactionId];
                transactionLock = Tuple.Create(transactionLock.Item1 - 1, transactionLock.Item2);

                if (0 == transactionLock.Item1)
                {
                    transactionService.transactionLockMap.Remove(transactionId);
                }
                else
                {
                    transactionService.transactionLockMap[transactionId] = transactionLock;
                }
            }

            Monitor.Exit(transactionLock.Item2);
        }
    }
}

The idea is that processing thread will use transactionService.AcquireTransactionLock(transactionId) in using block to synchronize transaction processing code.

Usage demo:

public class EntryPoint
{
    public static int Main()
    {
        var random = new Random();
        var transactionPersistenceContext = new TransactionPersistenceContext();
        var transactionService = new TransactionService();

        Action<object> transactionProcessor = (object parameter) =>
        {
            var transactionId = (int) parameter;

            for (int i = 0; i < 1000; i++)
            {
                using (transactionService.AcquireTransactionLock(transactionId))
                {
                    var transaction = transactionPersistenceContext.GetTransaction(transactionId);
                    transaction.Process();
                    Thread.Sleep(TimeSpan.FromMilliseconds(random.Next(10)));
                    transactionPersistenceContext.PersistTransaction(transaction);
                }
            }
        };

        var threads = Enumerable.
            Range(1, 8).
            Select(_ => new Thread(new ParameterizedThreadStart(transactionProcessor))).
            ToArray();

        for (var i = 0; i < threads.Length; i++)
        {
            threads[i].Start(0 == i % 2 ? 1 : 2);
        }

        foreach (var thread in threads)
        {
            thread.Join();
        }

        // Counters should count to 4000.
        var transaction1 = transactionPersistenceContext.GetTransaction(1);
        Console.WriteLine("transaction1.Counter: {0}.", transaction1.Counter);
        var transaction2 = transactionPersistenceContext.GetTransaction(2);
        Console.WriteLine("transaction2.Counter: {0}.", transaction2.Counter);
        return 0;
    }
}

Full code here: https://dotnetfiddle.net/ztd2Ar

Questions:

1. Is there any easier or more elegant way to do this?
2. Is there any synchronization issue present in this implementation?

Answer 1

class Transaction
{
    public int Id { get; set; }
    public int Counter { get; set; }
}

Neither of the properties should be publicly settable.

class TransactionService
{
    private readonly Dictionary<int, Tuple<int, object>> transactionLockMap; // id -> (referenceCount, lock).

    class TransactionLock : IDisposable
    {
            Tuple<int, object> transactionLock;
                    transactionLock = Tuple.Create(1, new object());

This is all a bit confusing because you actually have a class called TransactionLock but you're the using the same words for a concept of a transaction lock; transactionLockMap isn't a map of TransactionLocks; transactionLock isn't a TransactionLock.

transactionLock = Tuple.Create(transactionLock.Item1 + 1, transactionLock.Item2);

I don't like the usage of Tuples here. It would be clearer if you created a class which held the relevant items in well named properties.

lock (transactionService.syncRoot)
if (!transactionService.transactionLockMap.TryGetValue(transactionId, out transactionLock))

I don't like this reaching in to the internals of the TransactionService by another class. Sure it's a nested class but imo that's no excuse. The TransactionService should provide methods that can be called, which encapsulate the nitty gritty work on its internals.

Answer 2

I cannot verify this pattern for its correctnes but I find you can simplify and make the TransactionLocker better testable by removing the trasaction logic from it.

Let the Locker do only the locking and delegate the actions by using two lambdas: one for onLocked and the other one for onUnlocking.

class Locker : IDisposable
{   
    private readonly object _syncRoot;
    private readonly Func<object> _onLocked;
    private readonly Action _onUnlocking;
    private object _lockedObject;

    public Locker(object syncRoot, Func<object> onLocked, Action onUnlocking)
    {
        _syncRoot = syncRoot;
        _onLocked = onLocked;
        _onUnlocking = onUnlocking;     

        lock (_syncRoot)
        {
            _lockedObject = _onLocked();
        }

        Monitor.Enter(_lockedObject);
    }

    public void Dispose()
    {
        lock (_syncRoot)
        {
            _onUnlocking();
        }

        Monitor.Exit(_lockedObject);
    }
}

Now you can even reuse the Locker somewhere else because it's not longer tightly coupled to the transation.