Is this code [theoretically] thread-unsafe?

Question

I'm experiencing a strange deadlock in the code that I've written.

The idea is to implement an asynchronous operation whose Stop is synchronous -- the caller has to wait until it completes. I've simplified the part where real work is done to a simple property increment (++Value, see below); in reality though, a heavy COM component is invoked which is very sensitive to threads.

The deadlock I'm experiencing is in the Stop() method where I explicitly wait for a manual-reset event that identifies a completed operation.

Any ideas what I could have done wrong? The code should be self-contained and compilable on its own.

using System;
using System.ComponentModel;
using System.Threading;
using System.Windows.Forms;

using ThreadingTimer = System.Threading.Timer;

namespace CS_ManualResetEvent
{
    class AsyncOperation
    {
        ThreadingTimer   myTimer;                 //!< Receives periodic ticks on a ThreadPool thread and dispatches background worker.
        ManualResetEvent myBgWorkerShouldIterate; //!< Fired when background worker must run a subsequent iteration of its processing loop.
        ManualResetEvent myBgWorkerCompleted;     //!< Fired before the background worker routine exits.
        BackgroundWorker myBg;                    //!< Executes a background tasks
        int              myIsRunning;             //!< Nonzero if operation is active; otherwise, zero.

        public AsyncOperation()
        {
            var aTimerCallbac = new TimerCallback(Handler_Timer_Tick);
            myTimer = new ThreadingTimer(aTimerCallbac, null, Timeout.Infinite, 100);

            myBg = new BackgroundWorker();
            myBg.DoWork += new DoWorkEventHandler(Handler_BgWorker_DoWork);

            myBgWorkerShouldIterate = new ManualResetEvent(false);
            myBgWorkerCompleted = new ManualResetEvent(false);
        }

        public int Value { get; set; }

        /// <summary>Begins an asynchronous operation.</summary>
        public void Start()
        {
            Interlocked.Exchange(ref myIsRunning, 1);

            myTimer.Change(0, 100);

            myBg.RunWorkerAsync(null);
        }

        /// <summary>Stops the worker thread and waits until it finishes.</summary>
        public void Stop()
        {
            Interlocked.Exchange(ref myIsRunning, 0);

            myTimer.Change(-1, Timeout.Infinite);

            // fire the event once more so that the background worker can finish
            myBgWorkerShouldIterate.Set();

            // Wait until the operation completes; DEADLOCK occurs HERE!!!
            myBgWorkerCompleted.WaitOne();

            // Restore the state of events so that we could possibly re-run an existing component.
            myBgWorkerCompleted.Reset();
            myBgWorkerShouldIterate.Reset();
        }

        void Handler_BgWorker_DoWork(object sender, EventArgs theArgs)
        {
            while (true)
            {
                myBgWorkerShouldIterate.WaitOne();

                if (myIsRunning == 0)
                {
                    //Thread.Sleep(5000);   //What if it takes some noticeable time to finish?

                    myBgWorkerCompleted.Set();

                    break;
                }

                // pretend we're doing some valuable work
                ++Value;

                // The event will be set back in Handler_Timer_Tick or when the background worker should finish
                myBgWorkerShouldIterate.Reset();
            }

            // exit
        }

        /// <summary>Processes tick events from a timer on a dedicated (thread pool) thread.</summary>
        void Handler_Timer_Tick(object state)
        {
            // Let the asynchronous operation run its course.
            myBgWorkerShouldIterate.Set();
        }
    }

    public partial class Form1 : Form
    {
        private AsyncOperation myRec;
        private Button btnStart;
        private Button btnStop;

        public Form1()
        {
            InitializeComponent();
        }

        private void Handler_StartButton_Click(object sender, EventArgs e)
        {
            myRec = new AsyncOperation();
            myRec.Start();

            btnStart.Enabled = false;
            btnStop.Enabled = true;
        }

        private void Handler_StopButton_Click(object sender, EventArgs e)
        {
            myRec.Stop();

            // Display the result of the asynchronous operation.
            MessageBox.Show (myRec.Value.ToString() );

            btnStart.Enabled = true;
            btnStop.Enabled = false;
        }

        private void InitializeComponent()
        {
            btnStart = new Button();
            btnStop  = new Button();
            SuspendLayout();

            // btnStart
            btnStart.Location = new System.Drawing.Point(35, 16);
            btnStart.Size = new System.Drawing.Size(97, 63);
            btnStart.Text = "Start";
            btnStart.Click += new System.EventHandler(Handler_StartButton_Click);

            // btnStop
            btnStop.Enabled = false;
            btnStop.Location = new System.Drawing.Point(138, 16);
            btnStop.Size = new System.Drawing.Size(103, 63);
            btnStop.Text = "Stop";
            btnStop.Click += new System.EventHandler(Handler_StopButton_Click);

            // Form1
            ClientSize = new System.Drawing.Size(284, 94);
            Controls.Add(this.btnStop);
            Controls.Add(this.btnStart);
            Text = "Form1";
            ResumeLayout(false);
        }
    }

    static class Program
    {
        /// <summary>
        /// The main entry point for the application.
        /// </summary>
        [STAThread]
        static void Main()
        {
            Application.EnableVisualStyles();
            Application.SetCompatibleTextRenderingDefault(false);
            Application.Run(new Form1());
        }
    }
}

Answer 1

It seems like all you're trying to do is have an asynchronous task that starts with the press of a button and stops when another button is pressed. Given that, you seem to be over-complicating the task. Consider using something designed for cancelling an asynchronous operation, such as a CancellationToken. The async task simply needs to check the cancellation token's status in the while loop (as opposed to while(true)) and the stop method becomes as simple as calling cancel on the CancellationTokenSource.

private CancellationTokenSource cancellationSource;
private Task asyncOperationCompleted;

private void button1_Click(object sender, EventArgs e)
{
    //cancel previously running operation before starting a new one
    if (cancellationSource != null)
    {
        cancellationSource.Cancel();
    }
    else //take out else if you want to restart here when `start` is pressed twice.
    {
        cancellationSource = new CancellationTokenSource();
        TaskCompletionSource<object> tcs = new TaskCompletionSource<object>();
        asyncOperationCompleted = tcs.Task;
        BackgroundWorker bgw = new BackgroundWorker();
        bgw.DoWork += (_, args) => DoWork(bgw, cancellationSource);
        bgw.ProgressChanged += (_, args) => label1.Text = args.ProgressPercentage.ToString();
        bgw.WorkerReportsProgress = true;
        bgw.RunWorkerCompleted += (_, args) => tcs.SetResult(true);

        bgw.RunWorkerAsync();
    }
}

private void DoWork(BackgroundWorker bgw, CancellationTokenSource cancellationSource)
{
    int i = 0;
    while (!cancellationSource.IsCancellationRequested)
    {
        Thread.Sleep(1000);//placeholder for real work
        bgw.ReportProgress(i++);
    }
}

private void StopAndWaitOnBackgroundTask()
{
    if (cancellationSource != null)
    {
        cancellationSource.Cancel();
        cancellationSource = null;

        asyncOperationCompleted.Wait();
    }
}

Answer 2

put this code under ++Value; in Handler_BgWorker_DoWork. Then press the button when you see the output in debug window. Deadlock occurs then.

            int i = 0;
            while (i++ < 100) {
                System.Diagnostics.Debug.Print("Press the button now");

                Thread.Sleep(300);
                Application.DoEvents();
            }