Should I always add CancellationToken to my controller actions?

Question

Is this a good practice to always add CancellationToken in my actions no matter if operation is long or not?

I'm currently adding it to every action and I don't know if it's right or wrong.

[ApiController]
[Route("api/[controller]")]
public class DummiesController : ControllerBase
{
    private readonly AppDbContext _dbContext;

    public DummyController(AppDbContext dbContext)
    {
        _dbContext = dbContext;
    }

    [HttpGet("{id}")]
    public async Task<ActionResult<Dummy>> GetAsync(int id, CancellationToken ct) // <<----- should I always do this?
    {
        var dummy = await _dbContext.Dummies.AsNoTracking().SingleOrDefaultAsync(e=>e.Id == id, ct);
        if (dummy == null) return NotFound();
        return dummy;
    }
}

Also is adding CancellationToken ct = default(CancellationToken) necessary?

Add it if you are going to use it... If you don't plan on cancelling the operation, then why would you add it? — FCin, May 14 '18 at 11:57
@FCin I don't know. Maybe it would add some performance if user refreshes the page before server can fetch it from the database? Or it would be too fast to cancel? — Konrad, May 14 '18 at 12:51
Passing `CancellationToken` object doesn't magically cancel anything by itself. You have to cancel it yourself. — FCin, May 14 '18 at 12:54
@FCin in ASP.NET Core controller actions it does: https://andrewlock.net/using-cancellationtokens-in-asp-net-core-mvc-controllers/ — Konrad, May 14 '18 at 12:55
"ASP.NET Core provides a mechanism for the web server (e.g. Kestrel) to signal when a request has been cancelled using a CancellationToken. This is exposed as HttpContext.RequestAborted, but you can also inject it automatically into your actions using model binding." — Konrad, May 14 '18 at 12:57
"MVC will automatically bind any CancellationToken parameters in an action method to the HttpContext.RequestAborted token, using the CancellationTokenModelBinder. This model binder is registered automatically when you call services.AddMvc() (or services.AddMvcCore()) in Startup.ConfigureServices()." — Konrad, May 14 '18 at 12:58
Ok, I didn't know that. It would be reasonable to add it to long running methods. If your method takes ~50ms, then it doesn't add any benefit. — FCin, May 14 '18 at 12:59
I don't get it. How is this opinion-based? The first two answers provide great insight with multiple references. The second best answer explains why it is a bad idea to always put cancellation tokens for actions and this is very important in some scenarios. — Alexei - check Codidact, Jan 29 '20 at 08:59
@Alexei I just see only one reopen vote, while I've voted as well — Alireza, Sep 05 '20 at 16:13
@Alireza It got closed this time and you can post your answer. I have also posted a simplified version of this question on [Codidact](https://software.codidact.com/questions/277946) which is more tolerant of this type of questions (i.e. design choices which might also rely on one's opinion). — Alexei - check Codidact, Sep 06 '20 at 13:45

score 21 · Answer 1 · edited Jan 29 '20 at 09:03

21

Should I always add CancellationToken to my controller actions?

No. You should not always.

Using CancellationTokens in ASP.NET Core MVC controllers
https://andrewlock.net/using-cancellationtokens-in-asp-net-core-mvc-controllers/

Whether this is correct behaviour will depend on your app. If the request modifies state, then you may not want to halt execution mid-way through a method. On the other hand, if the request has no side-effects, then you probably want to stop the (presumably expensive) action as soon as you can.

So if you have a method/action like below (simplified);

await ProcessOrder();
await UpdateInventory();

You do not want to cancel the order while it is being processed, if so, order can be completed, but you will not update the inventory if user passing through the tunnel, and loses the internet connection.

This is especially important when operations cannot be included in a Unit of Work like pattern (e.g. distributed system) and one should try to minimize cancellations.

edited Jan 29 '20 at 09:03

Alexei - check Codidact

17,850
12
118
126

answered Oct 24 '19 at 05:12

Teoman shipahi

43,086
13
113
137

2

Or you can use a transaction spanning both ProcessOrder and UpdateInventory. That way the request can still be cancelled if it's taking too long (and the client has retry / timeout policies configured for resilience), and the transaction will be rolled back leaving your database in a consistent state. – Jorn Vanloofsvelt Jan 24 '20 at 10:03
1

What if you are calling 3rd party api? Or doing I/O? – Teoman shipahi Jan 24 '20 at 14:31
1

Then you cannot guarantee consistency anyway, unless you have some cross-service transactional system going on (maybe event/callback-based with rollbacks on each service). – Jorn Vanloofsvelt Jan 24 '20 at 15:06
Then no need to use cancellations token for that. Just run to completion. – Teoman shipahi Jan 24 '20 at 15:08
1

Unless it's a resource intensive operation that you want to be cancelled when no one is waiting for a response. So let's just say it depends. – Jorn Vanloofsvelt Jan 24 '20 at 18:49
Resource intensive operations should be queued, and should be executed by separate processes. – Teoman shipahi Jan 24 '20 at 18:50
Maybe the calls to the API are not resource intensive themselves, but cancelling them might propagate the cancellation event to the second API. – Jorn Vanloofsvelt Jan 24 '20 at 19:02
"Might be". That can be a service there you don't have control of. Such as PayPal api, another payment/inventory system like Amazon etc. – Teoman shipahi Jan 24 '20 at 20:33
Like I said, it depends. But remember you can not guarantee consistency in this case. – Jorn Vanloofsvelt Jan 25 '20 at 16:25
This is not the point of neither the question nor the answer. – Teoman shipahi Jan 25 '20 at 18:23
I was adding nuance to your answer because you made it seem like consistency matters while your proposed solution is not guaranteeing it – Jorn Vanloofsvelt Jan 25 '20 at 18:52
You keep dragging the discussion out of the context. Consistency is a huge topic itself, and your suggestion against relational DBs with transactions is just a drop in an ocean. I did not mention it guarantees at all. I was referring the article with a dummy analogy. – Teoman shipahi Jan 25 '20 at 19:02
You are clearly talking about the inventory being consistent with the orders. So is the article you linked. It's not out of context – Jorn Vanloofsvelt Jan 25 '20 at 19:10
Please post another answer if you think this one is not just not enough. I really would like to see and learn from your perspective. I am not here to discuss consistency algorithms and it’s implementations, therefore we will need to c/p entire white papers here. Thank you! – Teoman shipahi Jan 25 '20 at 19:14
Unfortunately, the question is closed (hopefully it gets re-opened soon), so I cannot expand the point in a comment very well. Here is the idea: imagine a user has called your api that runs a long-running db update via EFCore and you've passed `CancellationToken` to `SaveChanges` method of the context. If the token signals a cancellation (happens in application shutdown, HTTP connection abortion, tab closing, etc) then the running db transaction gets rolled back and an exception is thrown. Now, you should think what you must do in case an exception occurs from a DB operation. – Alireza Sep 05 '20 at 15:17

score 16 · Answer 2 · answered Jan 15 '19 at 12:44

It's worthwhile adding if you have any dependency on an external resource.

Let's say your database is busy, or you have transient error handling / retry policies in place for your database connection. If an end user presses stop in their browser the cancellation token will aid any waiting operations (in your code) to cancel gracefully.

Think of it less about the long running nature under normal circumstances, but long running nature in less than ideal circumstances e.g. if running in Azure and your SQL database is undergoing routine maintenance and entity framework is configured to retry itself (asuming Entity framework responds to cancellation tokens sensibly).

As a side note: Polly resilience framework has excellent support for external cancellation tokens to coordinate retry cancellations from an external cancellation. Their wiki is worthwhile a read as it's a great eye opener to coordinating cancellation: https://github.com/App-vNext/Polly/wiki

Regarding CancellationToken ct = default(CancellationToken), it's probably more worthwhile on library code than on a Controller action. But handy if you are unit testing your controller directly but don't want to pass in a cancellation token. Having said that .net core's WebHostBuilder testframework is easier to test against than testing controller actions directly these days.

Concerning unit testing the action method code that takes a CancellationToken, unless you are testing the cancellation itself some how just pass in CancellationToken.None. — bytedev, Jan 22 '20 at 08:17
That's true unit testing shouldn't be the only reason `default(CancellationToken)` should be present. — Alex KeySmith, Jan 22 '20 at 12:20
Consider integration testing instead of unit testing your controllers. ASP.NET Core have great functionality for integration testing (which tests routing, model binding, authorization, etc). — Fred, Jul 09 '20 at 08:17
Thanks Fred yes I totally agree, it's unlikely you'd want to unit test a controller these days, it's probably a slip of the tongue, I probably meant testing in general. — Alex KeySmith, Jul 09 '20 at 14:18

Should I always add CancellationToken to my controller actions?

2 Answers2