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.
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.
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.
CancellationTokenSource
to cancel all tasks when the form is closingThis 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.
Async void methods can wreak havoc if the caller isn't expecting them to be async. When the return type is Task, the caller knows it's dealing with a future operation; when the return type is void, the caller might assume the method is complete by the time it returns.
The await expression causes async function execution to pause until a promise is settled (that is, fulfilled or rejected), and to resume execution of the async function after fulfillment.
When using await, it's going to unwrap the first exception and return it, that's why we don't hit the catch (AggregateException e) line. But if we use something like the below code sample, we catch the AggregateException , note that it's not a good idea since we're blocking the running thread.
The main benefits of asynchronous programming using async / await include the following: Increase the performance and responsiveness of your application, particularly when you have long-running operations that do not require to block the execution.
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
.
If you love us? You can donate to us via Paypal or buy me a coffee so we can maintain and grow! Thank you!
Donate Us With