SynchronizationContext.Send breaks lock statement

Question

In the following code, the CheckCounter method is displayed sometimes even the mCounter variable is modified only within lock statement. If I comment the DoAnythingElseWithUI calling, the problem never occurs. It seems that DoAnythingElseWithUI breaks the lock statement and allow the timer1 event continue before the timer2 event release the lock.

using System;
using System.Collections.Generic;
using System.ComponentModel;
using System.Data;
using System.Drawing;
using System.Linq;
using System.Text;
using System.Threading;
using System.Threading.Tasks;
using System.Timers;
using System.Windows.Forms;

namespace LockThreadTest
{
    public partial class Form1 : Form
    {
        private SynchronizationContext mUIContext;
        private System.Timers.Timer mTimer1 = new System.Timers.Timer();
        private System.Timers.Timer mTimer2 = new System.Timers.Timer();

        public delegate void CompletedEventHandler(object sender);
        public event CompletedEventHandler CompletedEvent1;
        public event CompletedEventHandler CompletedEvent2;

        private object lockObject = new object();

        private static int mCounter = 0;

        public Form1()
        {
            mUIContext = WindowsFormsSynchronizationContext.Current;

            InitializeComponent();

            mTimer1.Interval = 1000;
            mTimer2.Interval = 1000;
            mTimer1.Elapsed += new System.Timers.ElapsedEventHandler(Timer1_Elapsed);
            mTimer2.Elapsed += new System.Timers.ElapsedEventHandler(Timer2_Elapsed);

            this.CompletedEvent1 += Form1_CompletedEvent1;
            this.CompletedEvent2 += Form1_CompletedEvent2;
        }

        public virtual void OnCompletedEvent1()
        {
            if (CompletedEvent1 != null)
                CompletedEvent1(this);
        }

        public virtual void OnCompletedEvent2()
        {
            if (CompletedEvent2 != null)
                CompletedEvent2(this);
        }

        private void Timer1_Elapsed(object sender, System.Timers.ElapsedEventArgs e)
        {
            OnCompletedEvent1();
        }

        private void Timer2_Elapsed(object sender, System.Timers.ElapsedEventArgs e)
        {
            OnCompletedEvent2();
        }

        private void ProcessTimer1()
        {
            lock (lockObject)
            {
                CheckCounter();

                mCounter++;
                WriteLog($"ProcessTimer1 started {mCounter}");

                Random random = new Random();
                Thread.Sleep(random.Next(50));

                mCounter--;
                WriteLog($"ProcessTimer1 finished {mCounter}");
            }
        }

        private void ProcessTimer2()
        {
            lock (lockObject)
            {
                CheckCounter();

                mCounter++;
                WriteLog($"ProcessTimer2 started {mCounter}");

                Random random = new Random();
                Thread.Sleep(random.Next(300));
                DoAnythingElseWithUI(); //after comment this line, the problem is gone

                mCounter--;
                WriteLog($"ProcessTimer2 finished {mCounter}");
            }
        }

        private void DoAnythingElseWithUI()
        {
            ExecuteUIContextAction(() =>
            {
                WriteLog("Anything else");
            });
        }

        private void CheckCounter()
        {
            if (mCounter != 0)
            {
                MessageBox.Show($"Alert! {mCounter}");
            }
        }

        private void WriteLog(string message)
        {
            richTextBox1.AppendText($"{DateTime.Now.ToString("HH:mm:ss:fffff")} {message}{Environment.NewLine}");
        }

        private void Form1_CompletedEvent1(object sender)
        {
            ExecuteUIContextAction(() =>
            {
                ProcessTimer1();
            });
        }

        private void Form1_CompletedEvent2(object sender)
        {
            ExecuteUIContextAction(() =>
            {
                ProcessTimer2();
            });
        }

        private void btnStart_Click(object sender, EventArgs e)
        {
            ThreadPool.QueueUserWorkItem(state =>
            {
                mTimer1.Start();
                mTimer2.Start();
            });
        }

        public void ExecuteUIContextAction(Action action)
        {
            if (mUIContext == null)
            {
                if (WindowsFormsSynchronizationContext.Current == null)
                {
                    WindowsFormsSynchronizationContext.SetSynchronizationContext(new SynchronizationContext());
                    mUIContext = WindowsFormsSynchronizationContext.Current;
                }
            }
            mUIContext.Send(new SendOrPostCallback(delegate (object state)
            {
                action();
            }), null);
        }

        public SynchronizationContext UIContext
        {
            get { return mUIContext; }
        }

        private void btnStop_Click(object sender, EventArgs e)
        {
            mTimer1.Stop();
            mTimer2.Stop();
        }
    }
}

Could somebody tell me why?

Is the right solution move the lock into Form1_CompletedEventX like this?

lock (lockObjectForRevisible)
{
   ExecuteUIContextAction(() =>
   {
      ProcessTimer1();
   });
}

Answer 1

There is a whole bunch of code with timers and stuff, but If I understand the code correctly, both ProcessTimer1 and ProcessTimer2 will run on the UI thread after some delay. And that the problem you are experiencing is that both method are somehow running at the same time.

An important feature of locks is that a single thread can take the same lock object multiple times:

var obj = new object();
lock(obj){
   lock(obj){
      // works fine!
   }
}

This is usually not a problem, since locks are meant to protect against multi threaded access, and as long as we are on a single thread all is usually fine.

I would guess that the reason for your issues is that when SynchronizationContext.Send is called from the UI thread is probably processes messages on the message queue. The UI thread has a queue of messages to do different things, draw the UI, process mouse/keyboard events, or run some arbitrary piece of code. SynchronizationContext.Send is one of the ways to add messages to this queue to run some arbitrary code. If I understand the documentation correctly, SynchronizationContext.Send is supposed to wait for the message to be processed before returning, and when called on the UI thread it needs to process pending messages unless you want a deadlock. So the order of events should be something like this:

1. ProcessTimer2 is called
2. ProcessTimer2 sleeps, and thus blocks the UI thread.
3. A message is added, asking the UI thread to call ProcessTimer1
4. ProcessTimer2 wakes up
5. ProcessTimer2 calls mUIContext.Send
6. mUIContext.Send processes messages, and calls ProcessTimer1
7. ProcessTimer1 observes that mCounter != 0

If you remove the DoAnythingElseWithUI call, mUIContext.Send will not be called, and ProcessTimer1 cannot start running until ProcessTimer2 returns control back to the message loop.

A fix is not easy to suggest, since it is not clear what you are trying to do. But for most applications you should avoid using anything other than the UI thread:

1. Use a windows forms timer that invokes event on the UI thread
2. Get rid of Thread.Sleep, if you really need to delay something, Task.Delay may be an alternative.
3. Get rid of the locks, since you should only be running code on the UI thread
4. Get rid of the synchronization context, since you should only be running code on the UI thread.

If you absolutely need to run some compute heavy code in the background, use await Task.Run(...)