Using lock in multiple threads with a stored copy of the lock object-CodePudding

I am trying to use lock with a shared object which I pass into my worker thread. In the code below, if I pass in the syncLock object in the Execute method of Worker, everything works fine.

However, if I store a local copy of the syncLock object in my Worker class, it does not work.

Obviously when I'm doing the "_syncLock = syncLock;" assignment, instead of having a reference to the shared syncLock object, I'm getting a new object. So I end up with each thread having it's own syncLock now instead of the shared object.

Is there way to store a local reference to the shared object? I thought that an object assignment is always a "reference" in C#?

Worker.cs

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading;
using System.Threading.Tasks;

namespace CacheConcurrency
{
    class Worker
    {
        int _ID;
        string _Request;
        object _syncLock;
        MyCache _TheCache;
        public Worker(int ID, string Request, ref object syncLock, ref MyCache TheCache)
        {
            _ID = ID;
            _Request = Request;
            _syncLock = syncLock;
            _TheCache = TheCache;
        }
        public void Execute()
        {
            lock (_syncLock)
            {
                if (_TheCache == null)
                {
                    Thread.Sleep(2000);
                    _TheCache = new MyCache();
                    _TheCache.LoadCache();
                    Console.WriteLine("thread {0}: created and loaded the cache", _ID);
                }
                else
                {
                    Console.WriteLine("thread {0}: using the existing cache", _ID);
                    Console.WriteLine("TheCache.MyCacheValue {0}", _TheCache.MyCacheValue);
                    Console.WriteLine("TheCache.CacheTimeStamp {0}", _TheCache.CacheTimeStamp);
                }
            }

            Console.WriteLine("worker DoSomething: {0}", _Request);
        }
    }
}

Main.cs

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading;
using System.Threading.Tasks;

namespace CacheConcurrency
{    
    class Main
    {
        public MyCache TheCache = null;
        public object syncLock = new object();

        public void Execute()
        {
            List<Task> TaskList = new List<Task>();

            for (int i = 0; i < 10; i  )
            {
                Task _Task = new Task(() => DoSomething(i, "test"));
                _Task.Start();
                Console.WriteLine("started Thread={0} at {1}", _Task.Id, DateTime.Now.ToString("hh:mm:ss.fff tt"));
            }

            Console.WriteLine("Press any key to quit the program");
            while (Console.ReadKey().KeyChar == 0) ;

        }

        void DoSomething(int ID, string Request)
        {
            Worker worker = new Worker(ID, Request, ref syncLock, ref TheCache);
            worker.Execute();
        }
    }
}

MyCache.cs

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading;
using System.Threading.Tasks;

namespace CacheConcurrency
{
    class MyCache
    {
        public int MyCacheValue;
        public DateTime CacheTimeStamp;
        public MyCache()
        {
            
        }

        public void LoadCache()
        {
            MyCacheValue = 1;
            CacheTimeStamp = DateTime.Now;            
        }
    }
}

Program.cs

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;

namespace CacheConcurrency
{
    class Program
    {
        static void Main(string[] args)
        {
            Main main = new Main();
            main.Execute();
        }
    }


}

CodePudding user response：

I worked out the problem, thanks for the comments who got me on the right track. In particular, thanks to MickyD who commented on the code needing to be thread-safe which led me to the answer.

It turns out that I was protecting with lock correctly in my Execute function, however in my constructor I was still referencing the shared Cache object without a lock, which was not thread-safe and causing a race condition.

The fix is to put both the constructor call and the Execute inside the critical section/lock block like so:

void DoSomething(int ID, string Request)
{
    lock(syncLock) 
    {
        Worker worker = new Worker();
        worker.Execute(ID, Request, ref TheCache);
    }
}

This also simplifies the worker code, avoiding the need to store a local reference of the syncLock object and doing a lock in there.

I also agree with MickyD's other comments that this code should be re-written to use async/await, not have a cache inside the worker etc. The longevity of us using this code base is in question, so investing in the tech debt is uncertain at the moment.