Multithreading problem (.NET)?

Question

.NET

What is the problem in the code? How to change the code to avoid it?

using System;
using System.Threading;

namespace Mover
{
    internal class Endpoint
    {
        public int Amount { get; set; }
    }

    internal class Program
    {
        private static void Main(string[] args)
        {
            var source = new Endpoint();
            var target = new Endpoint();

            var initialAmount = 1000000;
            source.Amount = initialAmount;

            var thread = new Thread(new ThreadStart(delegate
                                                    {
                                                        Transfer(source, target, initialAmount);
                                                    }));
            thread.Start();
            Transfer(target, source, initialAmount / 2);
            thread.Join();

            Console.Out.WriteLine("source.Amount = {0}", source.Amount);
            Console.Out.WriteLine("target.Amount = {0}", target.Amount);
        }

        private static void Transfer(Endpoint source, Endpoint target, int count)
        {
            while (count-- > 0)
                lock (target)
                    lock (source)
                    {
                        source.Amount--;
                        target.Amount++;
                    }
        }
    }
}

Answer 1

A deadlock may result if one of the locks is executed in the main thread, and then control is given to another thread.
There are several ways to avoid it (here the task is very muddy, the goal is not clear, so it’s hard to say how it’s right)
1. make a common object-lock
2. put the lock in the setter of the Amount property
3. lock on objects in a certain sequence; for this, for example, you can enter the id of objects

Answer 2

I'll offer my solution:

    public class Endpoint
    {
        private Mutex _sendLock;
        private Mutex _receiveLock;

        private int _amount;
        public int Amount
        {
            get
            {
                return _amount;
            }
        }

        public Endpoint(int amount)
        {
            _sendLock = new Mutex();
            _receiveLock = new Mutex();

            _amount = amount;
        }

        public static void Transmit(Endpoint source, Endpoint destination, int amount)
        {
            WaitHandle[] transmittingPartiesHandles = new WaitHandle[] { source._sendLock, destination._receiveLock };
            WaitHandle.WaitAll(transmittingPartiesHandles);

            // Do smth.

            Interlocked.Decrement(ref source._amount);
            Interlocked.Increment(ref destination._amount);

            source._sendLock.ReleaseMutex();
            destination._receiveLock.ReleaseMutex();
        }
    }

public class Program
    {
        public static void Main(string[] args)
        {
            Endpoint detroit = new Endpoint(1000000);
            Endpoint london = new Endpoint(0);

            int amount = detroit.Amount;

            Thread thread = new Thread(() => Transfer(detroit, london, amount));
            
            thread.Start();
            Transfer(london, detroit, amount / 2);
            thread.Join();

            Console.Out.WriteLine("source.Amount = {0}", detroit.Amount);
            Console.Out.WriteLine("target.Amount = {0}", london.Amount);
            Console.ReadLine();
        }

        private static void Transfer(Endpoint source, Endpoint target, int amount)
        {
            for (int i = 0; i < amount; i++)
            {
                Endpoint.Transmit(source, target, amount);
            }
        }
    }

First, we make the Endpoint object thread-safe by itself. After all, in fact, you cannot change the Amount property from external code.
Secondly, we get rid of locks on real objects (this is bad, because another scenario may exist in parallel, in which a lock on the same Endpoint object will again be taken for completely different purposes)
Thirdly, we separate the transfer process into a separate method. A shared lock will not work here if multiple transfers are configured at once in different places (there will be many “shared” lock objects for the same pair of endpoints, which will cause problems).