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?

Solution 1

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.

Solution 2

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.

Solution 3

I think cancellation token should be used mostly for query type operation. Take CRUD (Create, Read, Update, Delete) operation as example, break it down based on CQRS (Command Query Responsibility Segregation).

• Command: Create, Update, Delete
• Query: Read

As you can see, for Query operation, it will not be a problem to cancel at any time. But for Command, you most probably don't want to cancel any of the operation. For example Create, imagine you are creating data for 2 separate databases, then got cancelled half way, it'll cause unsynchronized data.

Comments

[bytedev]

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.

[Alex KeySmith]

That's true unit testing shouldn't be the only reason default(CancellationToken) should be present.

[Jorn Vanloofsvelt]

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.

[Teoman shipahi]

What if you are calling 3rd party api? Or doing I/O?

[Jorn Vanloofsvelt]

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).

[Teoman shipahi]

Then no need to use cancellations token for that. Just run to completion.

[Jorn Vanloofsvelt]

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.

[Teoman shipahi]

Resource intensive operations should be queued, and should be executed by separate processes.

[Jorn Vanloofsvelt]

Maybe the calls to the API are not resource intensive themselves, but cancelling them might propagate the cancellation event to the second API.

[Teoman shipahi]

"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.

[Jorn Vanloofsvelt]

Like I said, it depends. But remember you can not guarantee consistency in this case.

[Teoman shipahi]

This is not the point of neither the question nor the answer.

[Jorn Vanloofsvelt]

I was adding nuance to your answer because you made it seem like consistency matters while your proposed solution is not guaranteeing it

[Teoman shipahi]

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.

[Jorn Vanloofsvelt]

You are clearly talking about the inventory being consistent with the orders. So is the article you linked. It's not out of context

[Teoman shipahi]

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!

[Fred]

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).

[Alex KeySmith]

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.

[Alireza]

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.