0

In this answer the code is designed to have a delay between executions on a async task.

When I implement this I have a method that will cancel the token stopping the loop. I also wait on the task so that it is finished before I continue. If I don't catch the OperationCanceledException I will wait forever, but with it it works great.

I also tried replacing the while(true) with while (!cts.IsCancellationRequested) but it didn't stop the waiting.

Am I misunderstanding the usage?

private int count = 0;
private CancellationTokenSource cts;
private Task task;

public void ContinuousWorkTask(int interval)
{
  cts = new CancellationTokenSource();
  // Run the import task as a async task
  task = Task.Run(async () =>  // <- marked async
  {
    try {
      while (true) {
        DoWork(); 
        await Task.Delay(interval, cts.Token); // <- await with cancellation
      }
    }
    catch (OperationCanceledException) { }
  }, cts.Token);
}

private void DoWork()
{
  count++;
}

public void Shutdown()
{
  cts.Cancel();
  task.Wait(); //Without the try/catch we never stop waiting
}

Update #1: Show a very simple DoWork() to illustrate that it is not the reason the task "hangs" without the try/catch block.

Update #2: I like to wait on the task because I often have some sort of clean up to do on the way out such as writing a result to disk and I want to be sure it is done. I may do this as part of an IDisposable Dispose() method.

Update #3: It has been pointed out that the task.Wait() will receive an AggregateException. I think that adds support that the task should be catching the OperationCancelledException internally for a clean shutdown as I show in my example.

If it shouldn't I'd like to see an answer that explains why it is a better idea that I should be catching it from within the Shutdown() code. I think it is a code smell, but I may be missing a design point.

Community
  • 1
  • 1
Rich Shealer
  • 3,362
  • 1
  • 34
  • 59
  • 1
    Are you shure that `DoWork()` is not just takeing a long time to complete? If you don't pass `cts.Token` in to it you are going to have to wait till the last started `DoWork()` finishes before the `.Wait()` will stop waiting. – Scott Chamberlain Nov 09 '16 at 03:28
  • I have a very simple DoWork() for this test. (See updated question). Works fine without the `Try`/`Catch` block until I `Wait()` on the task after cancelling. I see a lot of examples without the `Try`/`Catch` so I wonder if I'm missing something. Maybe most programmers don't wait and the exception is swallowed – Rich Shealer Nov 09 '16 at 09:23
  • Without the try catch the `task.Wait()` call will throw an AggregateException. [It won't run indefinitely.](https://dotnetfiddle.net/PXPnwb) – default Nov 09 '16 at 09:30
  • What @Default says. The exception will be thrown from `task.Wait();`. – H H Nov 09 '16 at 09:33
  • Okay. If I `try`/`catch` around the `task,Wait()` I do get an `AggregateException` . and the program closes normally. This seems to tell me that their should be logic to cleanly catch the exception inside the task. – Rich Shealer Nov 09 '16 at 09:54
  • It tells me you should have a try/catch around the Wait. With `catch (AggregateException ex)` – H H Nov 09 '16 at 10:16
  • 1
    The bigger issue is why you initially missed that exception... – H H Nov 09 '16 at 10:24
  • I see a lot of example code like I linked to in my opening sentence that does not internally catch a expected **CancelledOperationException** and would require the external checking and I think it is a code smell to not handle the exception closer to the source.. – Rich Shealer Nov 09 '16 at 10:49
  • You don't have to write "**Update**" in your question. You are free to edit it however you like. If you want to see the edit history it's visible via the link in the middle bottom of your question. Besides that, what is your current question? You realized that `.Wait()` will throw, so your title is obsolete I guess. The question I see is "Is this a code smell", which to me is simply opinion based. Also, it's merely a comment. Could you perhaps edit your question to be a bit more focused on your current question instead of the thought progress that you have made? – default Nov 10 '16 at 08:39
  • I find that adding update helps people who are following the question see what was added quickly. – Rich Shealer Nov 10 '16 at 14:53
  • My current question hasn't changed. It seems to me the `try`/`catch` belongs in the task's loop and was left out as am implementation detail.. Without it I don't see a way to end the task gracefully. – Rich Shealer Nov 10 '16 at 14:58
  • "closer to the source" is not relevant. An exception should be handled when expected/understood and at the point where you can make the right decisions. Inside or outside the task cannot be decided for the provided pseudo case. – H H Nov 10 '16 at 20:25

0 Answers0