0

I have a timer on WinForm which I start when the form loads:

private void MainForm_Load(object sender, EventArgs e)    
{
    Action action = () => lblTime.Text = DateTime.Now.ToLongTimeString();

    Task task = new Task(() => {
        while (true)
        {
            Invoke(action);
            Task.Delay(1000);
        }
    });
    task.Start();
}

The problem is when I start the app in Debug mode in VS and the close it. I get an ObjectDisposedException which states that my form is already disposed.

I tried to fix it the following way:

private bool _runningTimer = true;

public MainForm()
{
    InitializeComponent();

    // ...
    FormClosing += MainForm_FormClosing;
}

private void MainForm_FormClosing(object sender, FormClosingEventArgs e)
{
    _runningTimer = false;
}

private void MainForm_Load(object sender, EventArgs e)
{
    Action action = () => lblTime.Text = DateTime.Now.ToLongTimeString();

    Task task = new Task(() => {
        while (_runningTimer)
        {
            Invoke(action);
            Task.Delay(1000);
        }
    });
    task.Start();
}

But the problem still ocurrs. What Am I doing wrong here?

UPDATE: I know that there is a standart timer for WinForms that works great in multithreaded invironment. I just wanted to know how it is possible to make it work to better understand how to deal with race conditions. This kind of timer is just an example, it could be another process that needs to update GUI.

UPDATE 2: Following the Hans Passant and Inigmativity answers I came to that code:

private void MainForm_Load(object sender, EventArgs e)
{
    Action action = () => { lblTime.Text = DateTime.Now.ToLongTimeString(); };

    Task task = new Task(async () => {
        while (!IsDisposed)
        {
            Invoke(action);
            await Task.Delay(1000);
        }
    });
    task.Start();
}

But anyway if I make time interval, for example 100ms, the ObjectDisposedException still throws.

This is not real life example, I just experimenting with it...

Dmitry Stepanov
  • 2,776
  • 8
  • 29
  • 45
  • 2
    The issue is that the form has closed inbetween `Invoke(action)` being called, and the Invoke posting the callback message to the message queue. It's a classic race condition. Unfortunately, this is quite hard to fix reliably. You might have to just catch and ignore that particular exception (*shudder*). Note that you can't do anything clever like try to wait for an in-flight Invoke to finish inside `MainForm_FormClosing()`, because then you can just end up with deadlock. – Matthew Watson Jun 20 '18 at 12:16
  • also my first impulse: displaying a datetime will not wreak havoc when just try..catch ignored. but consider using a timer (event-driven) to update your control. – Cee McSharpface Jun 20 '18 at 12:17
  • You need to stop those tasks before closing the form, or your tasks has to be aware of the form being closed/having been closed. The problem is that you're still trying to `Invoke(action)` on the form, *after* it has been closed, it's a simple timing issue. Since you haven't made `_runningTimer` volatile, the compiler is allowed to optimize this, but even making it volatile might not guarantee that this works. – Lasse V. Karlsen Jun 20 '18 at 12:18
  • Yes, a Window-based timer is probably the best way to go. – Matthew Watson Jun 20 '18 at 12:18
  • Why aren't you using a simple Winforms timer instead? – Lasse V. Karlsen Jun 20 '18 at 12:18
  • It is a fundamental threading race bug. You must avoid it by not allowing the window to close until *after* you ensured that the Task is no longer active and can no longer call Invoke(). [Look here](https://stackoverflow.com/a/1732361/17034) for sample code. The ODE is actually easy to avoid, the *action* needs to check if the form's IsDisposed property is false. But that doesn't also ensure that the task stops running, it does take *two* counter-measures. – Hans Passant Jun 20 '18 at 13:09
  • Don't allow the window to be closed until your timer stops. That's the only safe way. – Dharani Kumar Jun 20 '18 at 13:21
  • 2
    Do you know what `Task.Delay(1000);` does? Nothing. It doesn't wait 1 second. You have to either `await Task.Delay(1000);` or `Task.WaitAll(Task.Delay(1000));`. – Enigmativity Jun 20 '18 at 14:10
  • Thanks, you're right, but actualy this doesn't change too much because if I make time interval let's say 100 ms and await it, I'll get the same result with ObjectDisposedException. – Dmitry Stepanov Jun 21 '18 at 08:54

2 Answers2

0

In your first example the Task has no idea your app is exiting and is at risk of invoking the action after the label is destroyed, hence the ObjectDisposedException.

Though you attempt to alert the task in the second example, it isn't really that thread-safe and you could still invoke the action after the control is disposed.

Timers

A better solution is to just use a WinForms Timer. If you place the timer on the form via the designer, it automatically registers it as a component dependency making lifetime management much easier.

With WinForm timers you don't need to worry about threads or Tasks and more importantly you won't need to worry about Invoke if you need to update the UI (as opposed to child threads or non-UI context tasks)

Tell me more

  • "won't fire if the window is in the process of closing" - this isn't strictly true. If the timer has been attached to the forms components then the timer is shutdown when the form shuts down and that prevents it firing. There's nothing magical about timers that make them not fire if the form is closing. – Enigmativity Jun 20 '18 at 14:37
0

Ok, I tried to use task cancellation the following way:

public MainForm()
{
    InitializeComponent();
    cts = new CancellationTokenSource();

    Load += MainForm_Load;
    FormClosing += MainForm_FormClosing;
}

private void MainForm_FormClosing(object sender, FormClosingEventArgs e)
{
    cts.Cancel();
}

private void MainForm_Load(object sender, EventArgs e)
{        
    CancellationToken ct = cts.Token;

    Action action = () => { lblTime.Text = DateTime.Now.ToLongTimeString(); };

    var task = Task.Factory.StartNew(async () => {
        ct.ThrowIfCancellationRequested();

        while (true)
        {
            Invoke(action);
            await Task.Delay(100);
        }
    }, ct);
}

Don't know whether it's right but it seems works even if the time interval set to 10 ms.

Dmitry Stepanov
  • 2,776
  • 8
  • 29
  • 45