0

I have a piece of code which is doing something like:

 CancellationTokenSource _myCts;
 private async Task RunAsync()
 {
      using (_myCts = CancellationTokenSource.CreateLinkedTokenSource(token1, token2))
      {
          await DoWork();
      }
 }

 public void CancelOperation()
 {
     if (_myCts != null)
     {
        _myCts.Cancel();
     }
 }

It is correct this code? I am afraid that the Cancel() method of the cancellation token may be executed after disposed. It could be that the if checks the non-nullability of it, and right after that the using statement finishes.

Thank you

Cadmi
  • 53
  • 1
  • 7
  • Depending on how `DoWork` is implemented, you may have a race condition. The task may finish right after the moment something decides to call `CancelOperation()`. `myCts.Cancel()` would then throw `ObjectDisposedException`. The null check is also suspicious because the using statement will not set _myCts to null, you are left with a field that references a disposed object. – Wim Coenen May 19 '20 at 14:14
  • I would get rid of the CancelOperation method and _myCts field, and instead allow a cancellation token to be passed as an argument to the method that starts the asynchronous operation. – Wim Coenen May 19 '20 at 14:29
  • Thanks for your answer, i expected that.. The reason why i am using the cts, is because i have several tokens.. and it is more beautiful to just link them in one. An ugly solution would be to just try catch the Cancel just in case it is already disposed.. – Cadmi May 19 '20 at 14:43
  • `using` is only safe if the only place where `Cancel` is called is from inside the `using` block. Otherwise you have a race condition that is quite tricky to handle. Take a look at this question: [When to dispose CancellationTokenSource?](https://stackoverflow.com/questions/6960520/when-to-dispose-cancellationtokensource) – Theodor Zoulias May 19 '20 at 15:01
  • I see, thank you. I think using the lock statement could be one solution, but implementing the using clausule manually (try,catch,finally..) – Cadmi May 19 '20 at 15:09
  • @TheodorZoulias i also see in some examples that after calling Dispose on the cts, they are setting the reference to null. Is that necessary?¿ – Cadmi May 19 '20 at 15:26
  • Setting the reference to null allows the object to be garbage collected. It could also function as an indicator that the object is disposed, as an alternative to a `private bool _ctsIsDisposed` field. – Theodor Zoulias May 19 '20 at 15:30
  • This question needs more details in order to provide an answer. Why is it using a field instead of passing a `CancellationToken` to `Run`? It's not at all clear what this code is actually *trying* to do w.r.t. cancellation. – Stephen Cleary May 20 '20 at 20:33
  • @StephenCleary I think that's not relevant. We've got a class which needs to create a "cts" in the specified way; that is something quite common. And the question is if it is safe to use "using" when there is another thread which can request it to Cancel the operation. Don't you think so? – Cadmi May 22 '20 at 07:13

0 Answers0