RFC: Modernizing CancellationToken Usage in Async APIs

[josephdecock]

Hey everyone,
We're considering a significant change to how IdentityServer handles asynchronous operations, and we need your feedback before moving forward.

TL;DR: We want to add CancellationToken parameters to our async APIs. This is the standard, idiomatic C# approach, but it would be a major breaking change. We want to know if the benefits are worth the migration effort for you.

The Current Situation
Currently, most async APIs in IdentityServer don't accept a CancellationToken parameter. Instead, you resolve an ICancellationTokenProvider from DI. This provider gives you a token tied to the current HttpContext or CancellationToken.None if no request is active. We originally chose this dependency injection pattern to add cancellation support without breaking existing method signatures.

The Problem
While this approach avoided breaking changes, it has some clear downsides that many of you may have encountered:

• Inflexibility: You can't supply your own CancellationToken for a specific operation, for example, to implement a custom timeout.
• Background Jobs: Any code running outside of an HTTP request (like a cleanup job) always receives CancellationToken.None, which makes it impossible to cancel these operations gracefully.

The Proposal
We propose to refactor our async APIs to accept an optional CancellationToken parameter directly and remove the ICancellationTokenProvider completely.

// Before
Task DoSomethingAsync(string data);

// After
Task DoSomethingAsync(string data, CancellationToken cancellationToken = default);

We think this would be more intuitive and align with standard .NET practice. It would solve the problems mentioned above and give you control over cancellation.

The Big Trade-Off: Breaking Changes
Adding these parameters would be a major breaking change. Anyone implementing our interfaces or overriding async virtual methods of our default implementations would need to update their code to add the new parameter. This could have a cascading effect on your code.

We Need Your Feedback! 🤔
This is where you come in. We want to understand the real-world impact before we commit to such a large change.

• How would this breaking change affect your projects? Is it a minor inconvenience or a major migration headache?
• Have the current limitations of ICancellationTokenProvider been a problem for you? If so, can you share a specific scenario?
• Are the benefits of direct CancellationToken usage worth the pain of the breaking changes?
• Do you have any alternative solutions or ideas we should consider?

We appreciate you taking the time to share your thoughts. Your feedback will directly influence the future direction of IdentityServer!

[stefannikolei]

I want to keep it short. That's the right way to go. It probably will only be breaking for a small percentage of the userbase.

Does this proposal mean that you drop the idea of dropping the async suffix?

[gingters]

Completely second that. It's standard, it's a best practice and it should be done.

And in the simplest case, when you override the method you just need to add the parameter to the signature and be good, in the worst case, you need to adjust some lines in your method to simply pass the token along. These are all very small and straight forward refactorings that an AI assisted coding tool can do for you in a very short time. It's not a real issue, just a minor inconvenience.

[josephdecock]

Thanks for the feedback guys! While they are somewhat related, we're thinking of Cancellation Tokens and the async suffix as separate topics. For others' context, the idea of dropping the async suffix in method names is being discussed over here: https://github.com/orgs/DuendeSoftware/discussions/145. Feedback welcome there too!

[twenzel]

I'll also welcome this change. It's the "standard" and the normal behaviour a library user expects.

[RacerDelux]

This would be a much preferred change. While it is breaking for some, the "fix" in this case is pretty straight forward. I think it would be very worth the time to fix any issues to align better with standards in C#.

[kyle-sexton]

Yes, I think this would be a welcome change, as it is pretty much a standard convention I think most are familiar with. It would be pretty easy to fix any compile errors.

[aaronclawrence]

Could you give a list of what methods this would actually change, or at least some good examples, so I could check our code?

[josephdecock]

The full list of the changed methods is pretty big since every async method would get the new CancellationToken parameter, but here are some highlights. These are types that users commonly either implement or derive from, and all their async methods would change:

• IProfileService
• DefaultProfileService
• IResourceStore
• IClientStore
• IPersistedGrantStore

This is far from exhaustive, since the entire IdentityServer pipeline is extensible, but hopefully gives a sense of what would be involved.

[maartenba]

Related: https://github.com/orgs/DuendeSoftware/discussions/323

[SeanFarrow]

As someone who is implementing custom stores, this would be a welcome change. I'd like to be able to take a dependency on just the store interfaces and not have to worry about ICancellationTokenProvider as I do now.

Given that this will hopefully be done in a major version release, I don't see the fact that this is a breaking change as much of an issue.