14

Consider this code that runs on the UI thread:

dividends = await Database.GetDividends();
if (IsDisposed)
    return;
//Do expensive UI work here
earnings = await Database.GetEarnings();
if (IsDisposed)
    return;
//Do expensive UI work here
//etc...

Note that every time I await I also check IsDisposed. It's necessary because say I await on a long running Task. Meanwhile the user closes the form before it completes. The Task will finish and run a continuation that attempts to access controls on a disposed form. An exception occurs.

Is there a better way to handle this or simplify this pattern? I use await liberally in UI code and it's both ugly to check for IsDisposed every time and error prone if I forget.

EDIT:

There are a few proposed solutions that don't fit the bill because they change functionality.

  • Prevent form closing until background tasks complete

This will frustrate the users. And it also still allows potentially expensive GUI work to occur that is a waste of time, hurts performance and is no longer relevant. In the case where I'm almost always doing background work this could prevent the form close for a very long time.

  • Hide the form and close it once all tasks complete

This has all the problems of preventing the form close except doesn't frustrate users. The continuations that do expensive GUI work will still run. It also adds complexity of tracking when all tasks complete and then closing the form if it's hidden.

  • Use a CancellationTokenSource to cancel all tasks when the form is closing

This doesn't even address the problem. In fact, I already do this (no point in wasting background resources either). This isn't a solution because I still need to check IsDisposed due to an implicit race condition. The below code demonstrates the race condition.

public partial class NotMainForm : Form
{
    private readonly CancellationTokenSource tokenSource = new CancellationTokenSource();

    public NotMainForm()
    {
        InitializeComponent();
        FormClosing += (sender, args) => tokenSource.Cancel();
        Load += NotMainForm_Load;
        Shown += (sender, args) => Close();
    }

    async void NotMainForm_Load(object sender, EventArgs e)
    {
        await DoStuff();
    }

    private async Task DoStuff()
    {
        try
        {
            await Task.Run(() => SimulateBackgroundWork(tokenSource.Token), tokenSource.Token);
        }
        catch (TaskCanceledException)
        {
            return;
        }
        catch (OperationCanceledException)
        {
            return;
        }
        if (IsDisposed)
            throw new InvalidOperationException();
    }

    private void SimulateBackgroundWork(CancellationToken token)
    {
        Thread.Sleep(1);
        token.ThrowIfCancellationRequested();
    }
}

The race condition happens when the task has already completed, the form has closed, and the continuation still runs. You will see InvalidOperationException being thrown occasionally. Cancelling the task is good practice, sure, but it doesn't alleviate me from having to check IsDisposed.

CLARIFICATION

The original code example is exactly what I want in terms of functionality. It's just an ugly pattern and doing "await background work then update GUI" is a quite common use case. Technically speaking I just want the continuation to not run at all if the form is disposed. The example code does just that but not elegantly and is error prone (if I forget to check IsDisposed on every single await I'm introducing a bug). Ideally I want to write a wrapper, extension method, etc. that could encapsulate this basic design. But I can't think of a way to do this.

Also, I guess I must state performance is a first-class consideration. Throwing an exception, for example, is very expensive for reasons I won't get into. So I also don't want to just try catch ObjectDisposedException whenever I do an await. Even uglier code and also hurts performance. It seems like just doing an IsDisposed check every single time is the best solution but I wish there was a better way.

EDIT #2

Regarding performance - yes it is all relative. I understand the vast majority of developers don't care about the cost of throwing exceptions. The true cost of throwing an exception is off-subject. There is plenty of information available on this elsewhere. Suffice to say it's many orders of magnitude more expensive than the if (IsDisposed) check. For me, the cost of needlessly throwing exceptions is unacceptable. I say needless in this case because I already have a solution that doesn't throw exceptions. Again, letting a continuation throw an ObjectDisposedException is not an acceptable solution and exactly what I'm trying to avoid.

13
  • 1
    @HansPassant Sure, I could do that. But then I would just have a bunch of angry users waiting for a form to update that they want closed. Also, the example is greatly simplified. I could have many tasks running that captured the UI context at any given time. The above code is what I want functionally but now I have this "if not disposed continue" pattern infesting my code base.
    – Zer0
    Jun 26, 2015 at 0:43
  • 1
    @Loathing I thought about this too. But it adds the complexity of tracking running tasks. More importantly the UI code still ends up running (including subsequent awaits like above) which is expensive and wasteful in many ways. Checking a bool like you mentioned is the same as checking IsDisposed.
    – Zer0
    Jun 26, 2015 at 8:00
  • 3
    You seem to be seriously overestimating the performance costs of using an exception here. You have an exceptional situation, this type of behavior is exactly the type of things exceptions are here for. And throwing and then later catching one exception while the process is being torn down after the user has already closed the application shouldn't be a performance issue at all. Given that the UI is closed, the user isn't even likely going to realize that cleanup is still going on, or care if it takes an extra millisecond to finish.
    – Servy
    Jun 26, 2015 at 17:59
  • 2
    @Zer0 "very expensive" is a relative term. On a human timescale of the user of your application, the costs of one exception being thrown and captured are minuscule; not even perceptible. Now if you're constantly throwing and catching exceptions many tens of thousands of times a second, then perhaps it could become a relevant performance problem. Doing it once or twice when a form is closed is nowhere near that. Even if the form closing isn't the end of the application, it's still not going to impede the user experience in any way that a person is even going to be capable of perceiving.
    – Servy
    Jun 26, 2015 at 18:49
  • 2
    @Zer0 Heck, even your proposed solution of using task cancellation to solve this problem is using exceptions. It's throwing an exception when the work is being done, only to catch it later. It's going to have all of the same performance characteristics of the solution that you're completely disregarding for performance reasons.
    – Servy
    Jun 26, 2015 at 19:08

3 Answers 3

4

I also use IsDisposed to check the state of the control in such situations. Although it is a bit verbose, it is no more verbose than necessary to handle the situation - and it is not confusing at all. A functional language like F# with monads could probably help here - I'm no expert - but this seems as good as it gets in C#.

2
  • 1
    The object can become disposed in between the call to IsDisposed and using the object. Instead, the correct pattern is to try { ... } catch (ObjectDisposedException) { }
    – fjch1997
    Mar 20, 2018 at 3:15
  • 2
    @fjch1997 does control disposal not happen on the main UI thread where you'd be updating controls? If so, I'd have expected that while you are running any code on the UI thread, it'd either be disposed or not disposed.
    – Jeff B
    Apr 29, 2019 at 22:13
2

It should be pretty straightforward to have a CancellationTokenSource owned by your form, and have the form call Cancel when it is closed.

Then your async methods can observe the CancellationToken.

6
  • This is the right way to do it. Also any logic should be factored out of the form (and unit tested). The form event handlers should be bare and should call straight to async methods on a view model or service, passing the method the cancellation token.
    – jnm2
    Jun 26, 2015 at 1:47
  • 1
    I thought about this but was worried about a possible race condition. So on FormClosing I call Cancel on the token. But the Task has already completed and posted its continuation (the rest of the method after the await) to the captured SynchronizationContext. Which in this case is sitting in the message queue and will still run. I suppose I can easily test this by heavily saturating the message queue.
    – Zer0
    Jun 26, 2015 at 2:55
  • There is indeed an easy to reproduce race condition here. This is worse than both my design and the suggestion of preventing the form closing because now there's unhandled exceptions. I can post the code I used to test if you'd like.
    – Zer0
    Jun 26, 2015 at 3:43
  • @Zer0 It would be useful if you could update your question to include the fact you have considered this solution but found a problem with it. Jun 26, 2015 at 9:45
  • @Zer0: Yes, cancellation is reported using exceptions. So, you catch it. Bottom line is that if you have a method that depends on an object, you either have to extend the lifetime of that object or you have to check for the object being in an invalid state (whether through a two-line if (IsDisposed) or a single-line token.ThrowIfCancellationRequested()). There's no way around that, logically. If you want to hide this from the user, you can intercept the form close so that it is hidden but not disposed; that wouldn't cancel the actual operation though. Jun 26, 2015 at 12:00
0

I once solved a similar issue by not closing the form. Instead, I hid it at first and only really closed it when all outstanding work had completed. I had to track that work, of course, in form of Task variables.

I find this to be a clean solution because disposal issues do not arise at all. Yet, the user can immediately close the form.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.