Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Suggestion: Exposing Cancellation Tokens #20

Closed
seangwright opened this issue May 9, 2020 · 8 comments
Closed

Suggestion: Exposing Cancellation Tokens #20

seangwright opened this issue May 9, 2020 · 8 comments

Comments

@seangwright
Copy link

seangwright commented May 9, 2020

Current State

Currently, if developers want to control operation cancellation through CancellationToken, they are a little stuck, since the HandleAsync() method doesn't have a CancellationToken parameter.

MVC auto injects these values if they are present.

Motivation

In an app that renders HTML for the browser, this might not be as important, but in client side JavaScript applications, it's not uncommon for for a user to interact with multiple parts of the UI in quick succession (like typing in a search box) that would cause previous XHR requests to be canceled since those responses would be discarded by the client anyway - it's only interested in the latest response.

ASP.NET Core will still handle the cancellation and not send the response, but the developer's pipeline of operations continue to execute on the previous request thread while the newer request is handled.

Many external service libraries (DB, HTTP) accept CancellationTokens to halt operations.

Solution

It would be nice to be able to support cancellation from end-to-end with ApiEndpoints.

I believe we'd need to change the HandleAsync() API to accept a CancellationToken token as the last parameter, which means a breaking change.

Thoughts

I'd be willing to PR this if you are interested in the idea.

I'm also wondering, since it would be a breaking change, if there were any other solutions to problems you'd like to roll in with it, like supporting multiple method params (not sure how... maybe multiple generics like the BCL does it with Func<> and Action<>?)

@JoeyMckenzie
Copy link
Contributor

After listening to Steve's episode about the project on .NET Rocks this past weekend, when he compared it to MediatR, this was the first thing that popped into my head. Considering most library providers support some form of CancellationToken in their service contracts, I think this would be worth the change. I've created a fork with the changes here, and before making the PR, I'd like input from the maintainers about the viability of this change.

To me as a consumer, it seems strange in the first place to not offer CancellationToken support on the async endpoint, and would consider this worth the contract breaking change for other consumers of the library. As the endpoints themselves are direct descendants from ControllerBase, we essentially get a free token source generator that we should be able to utilize in inherited endpoints.

@ardalis I'm curious of your thoughts and the viability of this change, and if you have any input, I'm all ears.

@ardalis
Copy link
Owner

ardalis commented Jul 14, 2020

I'd be happy to add an optional CancellationToken on HandleAsync - that makes perfect sense. That should work, right?

@ardalis
Copy link
Owner

ardalis commented Jul 14, 2020

Also @seangwright sorry for not responding to this sooner. Juggling a lot these days. :)

@JoeyMckenzie
Copy link
Contributor

@ardalis Correct me if I'm wrong, but I don't think optional parameters work in abstract methods, as consumers will still be forced to override the method containing the CancellationToken. Even with the default CancellationToken parameter I've add in my fork, consumers are still required to override the HandleAsync method matching the signature containing the token, effectively breaking the contract as it currently stands. I'm open to any suggestions and will be hacking on this later as time permits.

@ardalis
Copy link
Owner

ardalis commented Jul 14, 2020

I think you're right. I'm fine with the breaking change. We can just rev the version to 2.0. I think it makes sense to expose them, despite the fact I don't often implement them in most of the apps I build (that expect requests to take < 500ms each).

If one of you wants to make a PR adding them I'd take it, thanks.

@ztsexton
Copy link

ztsexton commented Aug 5, 2020

When can we expect a NuGet package update? If others try to mimic the sample application in master they won't be able to without realizing the latest NuGet package doesn't use cancellation tokens yet.

@ardalis
Copy link
Owner

ardalis commented Aug 5, 2020

I'll try and get an update out this week.

@ardalis
Copy link
Owner

ardalis commented Aug 7, 2020

@ardalis ardalis closed this as completed Aug 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants