Why does invoking an event in a timer callback cause following code to be ignored?

Question

I'm writing a simple game that uses timers from the system.threading namespace to simulate wait times for actions. My goal is to have the timer execute once every second for x amount of seconds. To achieve this I added a counter in the timer callback.

The problem is any code I place after invoking the DeliveryProgressChangedEvent event seems to get ignored. My counter is never incremented thus allowing the timer to run forever.

If I invoke the event after I increment the counter, everything works fine. Just nothing after invoking the event will execute. Rather than going the easy route I'd like to understand if not resolve this problem.

I did a fair bit of research into the system.threading timer object as well as events but wasn't able to find any information related to my issue.

I created a bare bones example of my project to demonstrate the issue below.

Game Class

    class Game
    {
        private Timer _deliveryTimer;
        private int _counter = 0;

        public event EventHandler DeliveryProgressChangedEvent;
        public event EventHandler DeliveryCompletedEvent;

        public Game()
        {
            _deliveryTimer = new Timer(MakeDelivery);
        }

        public void StartDelivery()
        {
            _deliveryTimer.Change(0, 1000);
        }

        private void MakeDelivery(object state)
        {
            if (_counter == 5)
            {
                _deliveryTimer.Change(0, Timeout.Infinite);
                DeliveryCompletedEvent?.Invoke(this, EventArgs.Empty);
            }

            DeliveryProgressChangedEvent?.Invoke(this, EventArgs.Empty);

            ++_counter;
        }
    }

Form Class

    public partial class Form1 : Form
    {
        Game _game = new Game();

        public Form1()
        {
            InitializeComponent();

            _game.DeliveryProgressChangedEvent += onDeliveryProgressChanged;
            _game.DeliveryCompletedEvent += onDeliveryCompleted;

            pbDelivery.Maximum = 5;
        }

        private void onDeliveryProgressChanged(object sender, EventArgs e)
        {
            if (InvokeRequired)
                pbDelivery.BeginInvoke((MethodInvoker)delegate { pbDelivery.Increment(1); });

            MessageBox.Show("Delivery Inprogress");
        }

        private void onDeliveryCompleted(object sender, EventArgs e)
        {
            MessageBox.Show("Delivery Completed");
        }

        private void button1_Click(object sender, EventArgs e)
        {
            _game.StartDelivery();
        }
    }

EDIT

Just to clarify what I mean. Any code I put after DeliveryProgressChangedEvent?.Invoke(this, EventArgs.Empty); will not execute. In my example ++_counter will not run. The event does fire and the onDeliveryProgressChanged handler does run.

Answer 1

The problem:
Using a System.Threading.Timer class, when the TimerCallback is called, events are raised, to notify the subscribers of the DeliveryProgressChangedEvent and DeliveryCompletedEvent of custom Game class of the progress of a procedure and the termination of it.

In the sample class, the subscriber (a Form class, here) updates an UI, settings the value of a ProgressBar control and also showing a MessageBox (used in the actual implementation of the class sample shown here).

It appears that after the first event is invoked:

DeliveryProgressChangedEvent?.Invoke(this, EventArgs.Empty);
++_counter;

the line where the _counter should be increased is never reached, thus the code that inspects the _counter to set the Timer to a new value is never executed.

What happens:

1. The System.Threading.Timer is served by ThreadPool threads (more than one). Its callback is called on a thread other than the UI thread. The events invoked from the callback are also raised in a ThreadPool thread.
   The code in the handler delegate, onDeliveryProgressChanged, is then run on the same Thread.

    private void onDeliveryProgressChanged(object sender, EventArgs e)
    { 
        if (InvokeRequired)
            pbDelivery.BeginInvoke((MethodInvoker)delegate { pbDelivery.Increment(1); });
        MessageBox.Show("Delivery Inprogress");
    }
   

   When the MessageBox is shown - it's a Modal Window - it blocks the Thread from where it's run, as usual. The code following the line where the event is invoked is never reached, so _counter is never increased:

    DeliveryProgressChangedEvent?.Invoke(this, EventArgs.Empty);
    ++_counter;
   

2. The System.Threading.Timer can be served by more than one thread. I'm just quoting the Docs on this point, it's quite straightforward:

   The callback method executed by the timer should be reentrant, because it is called on ThreadPool threads. The callback can be executed simultaneously on two thread pool threads if the timer interval is less than the time required to execute the callback, or if all thread pool threads are in use and the callback is queued multiple times.

   What happens, in practice, is that while the Thread where the CallBack is executed, is blocked by the MessageBox, this doesn't stop the Timer from executing the CallBack from another thread: a new MessageBox is shown when the event is invoked and it keeps on running until it has resources.

3. The MessageBox has no Owner. When a MessageBox is shown without specifying the Owner, its class uses GetActiveWindow() to find an Owner for the MessageBox Window. This function tries to return the handle of the active window attached to the calling thread's message queue. But the thread from which the MessageBox is run has no active Window, as a consequence, the Owner is the Desktop (actually, IntPtr.Zero here).

This can be manually verified by activating (clicking on) the Form where the MessageBox is called: the MessageBox Window will disappear under the Form, since it's not owned by it.

How to solve:

1. Of course, using another Timer. The System.Windows.Forms.Timer (WinForms) or the DispatcherTimer (WPF) are the natural substitutes. Their events are raised in the UI Thread.

► The code presented here is just a WinForms implementation made to reproduce a problem, hence these may not apply to all contexts.

2. Use a System.Timers.Timer: the SynchronizingObject property provides means to marshal the events back to the Thread that created the current class instance (same consideration in relation to the concrete implementation context).

3. Generate an AsyncOperation using the AsyncOperationManager.CreateOperation() method, then use a SendOrPostCallback delegate to let the AsyncOperation call the SynchronizationContext.Post() method (classic BackGroundWorker style).

4. BeginInvoke() the MessageBox, attaching it to the UI Thread SynchronizationContext. E.g.,:

    this.BeginInvoke(new Action(() => MessageBox.Show(this, "Delivery Completed")));
   

   Now the MessageBox is owned by the Form and it will behave as usual. The ThreadPool thread is free to continue: the Modal Window is synched with the UI Thread.

5. Avoid using a MessageBox for this kind of notifications, since it's really annoying :) There are many other ways to notify a User of status changes. The MessageBox is probably the less thoughtful.

To make them work as intended, without changing the current implementation, the Game and Form1 classes can be refactored like this:

class Game
{
    private System.Threading.Timer deliveryTimer = null;
    private int counter;

    public event EventHandler DeliveryProgressChangedEvent;
    public event EventHandler DeliveryCompletedEvent;

    public Game(int eventsCount) { counter = eventsCount; }

    public void StartDelivery() {
        deliveryTimer = new System.Threading.Timer(MakeDelivery);
        deliveryTimer.Change(1000, 1000);
    }

    public void StopDelivery() {
        deliveryTimer?.Dispose();
        deliveryTimer = null;
    }

    private void MakeDelivery(object state) {
        if (deliveryTimer is null) return;
        DeliveryProgressChangedEvent?.Invoke(this, EventArgs.Empty);
        counter -= 1;

        if (counter == 0) {
            deliveryTimer?.Dispose();
            deliveryTimer = null;
            DeliveryCompletedEvent?.Invoke(this, EventArgs.Empty);
        }
    }
}


public partial class Form1 : Form
{
    Game game = null;

    public Form1() {
        InitializeComponent();
        pbDelivery.Maximum = 5;

        game = new Game(pbDelivery.Maximum);
        game.DeliveryProgressChangedEvent += onDeliveryProgressChanged;
        game.DeliveryCompletedEvent += onDeliveryCompleted;
    }

    private void onDeliveryProgressChanged(object sender, EventArgs e)
    {
        this.BeginInvoke(new MethodInvoker(() => {
            pbDelivery.Increment(1);
            // This MessageBox is used to test the progression of the events and
            // to verify that the Dialog is now modal to the owner Form.  
            // Of course it's not used in an actual implentation.  
            MessageBox.Show(this, "Delivery In progress");
        }));
    }

    private void onDeliveryCompleted(object sender, EventArgs e)
    {
        this.BeginInvoke(new Action(() => MessageBox.Show(this, "Delivery Completed")));
    }

    private void button1_Click(object sender, EventArgs e)
    {
        game.StartDelivery();
    }
}

Answer 2

Good information. The callback method executed by the timer should be reentrant, because it is called on ThreadPool threads. The callback can be executed simultaneously on two thread pool threads if the timer interval is less than the time required to execute the callback, or if all thread pool threads are in use and the callback is queued multiple times.