C# lock to simultaneously read/write and display results

Question

here's my question:

Say I have this program (I'll try to semplify as much as I can): receiveResultThread waits for result from differents network clients, while displayResultToUIThread updates the UI with all the results received.

class Program
{
    private static Tests TestHolder;

    static void Main(string[] args)
    {
        TestHolder = new Tests();

        Thread receiveResultsThread = new Thread(ReceiveResult);
        receiveResultsThread.Start();

        Thread displayResultToUIThread = new Thread(DisplayResults);
        displayResultToUIThread.Start();

        Console.ReadKey();
    }

    public static void ReceiveResult()
    {
        while (true)
        {
            if (IsNewTestResultReceivedFromNetwork())
            {
                lock (Tests.testLock)
                    TestHolder.ExecutedTests.Add(new Test { Result = "OK" });
            }

            Thread.Sleep(200);
        }
    }

    private static void DisplayResults(object obj)
    {
        while (true)
        {
            lock (Tests.testLock)
            { 
                DisplayAllResultInUIGrid(TestHolder.ExecutedTests);
            }

            Thread.Sleep(200);
        }
    }
}

class Test
{
    public string Result { get; set; }
}

class Tests
{
    public static readonly object testLock = new object();
    public List<Test> ExecutedTests;

    public Tests()
    {
        ExecutedTests = new List<Test>();
    }
}

class UIManager
{
    public static void DisplayAllResultInUIGrid(List<Test> list)
    {
        //Code to update UI.
    }
}

Considering that the scope is to not update the UI while the other thread is adding tests to the list, it is safe to use:

lock (Tests.testLock)

or should I use:

lock (TestHolder.testLock)

(changing the static property of testLock)?

Do you think this is a good way to write this kind of program or can you suggest a better pattern?

Thank you for your help!

Answer 1

Public (not talking about public static) lock objects tend to be dangerous. Please see here

The reason it's bad practice to lock on a public object is that you can never be sure who ELSE is locking on that object.

Furthermore just having a List<T> and adding objects from an outer scope could be a smell, too.

In my opinion it'd be a better idea to have a method AddTest in Tests

class Tests
{
    private static readonly object testLock = new object();
    private List<Test> executedTests;

    public Tests()
    {
        ExecutedTests = new List<Test>();
    }

    public void AddTest(Test t)
    {
        lock(testLock)
        {
            executedTests.Add(t);
        }
    }

    public IEnumerable<Test> GetTests()
    {
        lock(testLock)
        {
            return executedTests.ToArray();
        }
    }
    [...]
}

Clients of your tests class do not have to worry about using the lock object correctly. Precisely, they don't have to worry about any of the internals of your class.

You could, anyway, rename your class to ConcurrentTestsCollectionor the like, that users of the class know, that it's thread safe to some extent.

Answer 2

While you can use Tasks and the async/await keywords to do this less verbosely, I don't think it will fully solve your question.

I will assume that ExecutedTests is a List(or like) that you want to be thread safe, which is why you are creating a lock while accessing it.

I would make the list, itself, thread safe, rather than the operations against it. This will remove the need for a lock or a lock object.

You could implement this yourself or use something in the System.Collections.Concurrent namespace.

P.S.

If the threads are meant to be closed(aborted) when the process is exited you should set the Thread's IsBackground property to true.