0

I understand that blocking on async code is disapproved in most cases.

However, I have a WPF application executing a background task. When the user closes the window, this task should be canceled and the closing of the window delayed until cancellation and subsequent cleanup has been completed. In the meantime, any further user input should be blocked.

After a bit of searching & trying, I came up with the following code:

ViewModel.cs:

async void RunTaskAsync()
{
    _cancelTknSource = new CancellationTokenSource();
    try {
        await Task.Run(/*...long running calculation...*/, _cancelTknSource.Token);
    }
    catch (OperationCanceledException) { }
    finally {
        /*...cleanup...*/
        _cancelTknSource.Dispose();
        _cancelTknSource = null;            
    }
}

async Task CancelAsync()
{
   _cancelTknSource?.Cancel();
   while (_cancelTknSource != null) {
       Thread.Sleep(10);
       await Task.Yield(); //prevents dead-lock
   }
}

Window.xaml.cs:

async void Window_Closing(object sender, CancelEventArgs e)
{
    e.Cancel = true;
    await DataContext.CancelAsync();
    Close();
}

(All these methods are called from the UI thread)

Is this good practice? If not, what are the alternatives?

Cornix
  • 19
  • 4
  • 2
    Welcome good sir. I wonder if this question might be better asked over at the sibling site _Code Review_ (because yours is a design rather than a problem question) on the StackExchange network? Good luck –  Dec 30 '17 at 16:53
  • 1
    Task.Yield() does not fix deadlock, it does not solve the fundamental issue that the UI thread is no longer capable of processing notifications. Advice for WPF is not fundamentally different [from Winforms](https://stackoverflow.com/a/1732361/17034). – Hans Passant Dec 30 '17 at 17:01
  • "await Task.Run(/*...long running calculation...*/,_cancelTknSource.Token);" if you are waiting for this task. What is the point of the token? – bichito Dec 30 '17 at 17:10
  • @Hans Passant: In my case, both the Run...() and the Cancel...() methods are called from the UI thread. So, without Task.Yield() the Run() method would wait indefinitely for the UI context in order to enter the catch/finally. – Cornix Dec 30 '17 at 17:12
  • @efekctive: The token is being checked periodically during the calculation task. – Cornix Dec 30 '17 at 17:15
  • 2
    the await does not buy you anything. After the task is created the only time you should worry it about is when cancelling. You are not processing the results from the task in RunTaskAsync(). Why await? – bichito Dec 30 '17 at 17:16
  • `await Task.Delay` would allow you to ditch the sleep call. Returning the cancellation token would seem better than keeping it encapsulated. Keeping it as class state makes it so that your class is not thread-safe. – Brannon Dec 30 '17 at 17:28
  • @Brannon: with Task.Delay, the UI stays responsive - allowing the user to interfere with the cleanup & closing - which I wanted to avoid. – Cornix Dec 30 '17 at 17:36
  • You might want to set some Continuation conditions for your scheduled Task (blocking or non blocking). See if [this](https://stackoverflow.com/questions/47723394/task-not-in-a-faulted-state-when-an-exception-is-thrown?answertab=active#tab-top) helps as an alternative solution. – Jimi Dec 30 '17 at 18:12
  • @Hans Passant: Just figured out that "from Winforms" was a link... – Cornix Dec 30 '17 at 20:07
  • Thanks for this alternative. Anyhow, setting IsEnabled=false in the Closing event handler does not disable all commands; so I probably need to add a few checks of IsBusy or IsCancellationPending. – Cornix Dec 30 '17 at 20:15
  • Especially lacking a good [mcve] that clearly illustrates your scenario, I don't think a good answer can really be written. But: first thing I'd change is your `RunTaskAsync()` method. It should return `Task`, you should save that `Task` in a field, and your `CancelAsync()` method should await that task (that would get rid of your terrible polling loop). The rest looks fine, not counting the endless `e.Cancel = true` loop you appear to have (i.e. you should only cancel the `Closing` event if you in fact do need to cancel and wait for the task). – Peter Duniho Dec 30 '17 at 20:37
  • @Peter Duniho: Thanks a lot. Just as Task.Delay, awaiting has the disadvantage of not blocking the UI, so I need to add a few more checks of IsBusy or IsCancellationRequested. Still, I think that is the way to go. – Cornix Dec 30 '17 at 21:28
  • @Peter Duniho: The endless loop is an artifact from rearranging and cutting down my production code... (obviously not a good way to obtain a minimal, complete and verifiable example) :-/ – Cornix Dec 30 '17 at 21:45
  • _"awaiting has the disadvantage of not blocking the UI"_ -- it does, but it would be better to use other mechanisms to block the UI in a non-blocking way. That is, you don't literally want the UI blocked, you want the user to not be able to interact with it (it should still redraw, window be draggable, etc.) You can disable the UI, or put a modal dialog above it, either of which are better options than literally just not processing any user input. – Peter Duniho Dec 31 '17 at 04:37
  • @Peter Duniho: I modified my code as you suggested. Most Buttons are disabled anyway while the task is running, so there was not much left to do. -- I discovered another downside of my original solution (apart from the UI freezing for about a second): When the Run...() method is called with lower DispatcherPriority than Cancel...(), the code deadlocks. – Cornix Dec 31 '17 at 14:14

1 Answers1

0

Without error handling and cleaning up(e.g. set _cancelTknSource and _runningTask to null), the following code should do. But you still have to block some UI features between Window_Closing and the actual closing of the window.

async void RunTaskAsync()
{
    using(_cancelTknSource = new CancellationTokenSource())
    {
        _runningTask =Task.Run(/*...long running calculation...*/, _cancelTknSource.Token);
        await _runningTask;
    }
}

Task CancelAsync()
{
   _cancelTknSource?.Cancel();
   return _runningTask;
}
Window.xaml.cs:

async void Window_Closing(object sender, CancelEventArgs e)
{
    e.Cancel = true;
    await DataContext.CancelAsync();
    Close();
}
Xiaoguo Ge
  • 2,177
  • 20
  • 26
  • To me, it is not obvious whether Close() will be executed before or after the remainder of RunTaskAsync after the using block (where I intend to do my cleanup) - because both methods are awaiting the same task... Maybe better change RunTaskAsync() from void to Task and store this in _runningTask (as Peter suggested) – Cornix Dec 30 '17 at 23:14
  • @Cornix regarding multiple `await`s on same task see [what happens if I await a task that is already running or ran?](https://stackoverflow.com/a/32434481/585968) –  Dec 31 '17 at 05:12