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
Clean up exposure and handling of CancellationToken #612
Conversation
- In public API, they should always be the last parameter (unless they can't be, e.g. due to a params array), named "cancellationToken", and be non-nullable. - Ensure they're forwarded appropriately from one API to the next. There may be some more missing cases, but I fixed all the ones I found. - Make the XML docs consistent. - Remove unnecessary cancellation-related analyzer suppressions One issue I noticed I haven't addressed here: - Some APIs take both an SKContext and a CancellationToken separately. If both are supplied, one of the tokens (either the explicitly passed one or the one from the SKContext) might be ignored. If they're not the same token and they're both cancelable, a combined token should really be passed on to the target so that the target ends up monitoring both.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to be reviewed offline
Todo: for ISKFunction and Plan it would be nice having the cancellation token as an optional parameter, without ever forcing the user to pass one. There are a lot of changes so I'll have to check the code locally. As a guideline, if the context is available, I would assume it has a valid cancellation token. If the context is created locally though, that's false so we need to check for that scenario. If the context is null and the user didn't pass a cancellation token I think we should log a warning (or an error). |
### Motivation and Context Clean up exposure and handling of CancellationToken in public APIs. ### Description - In public API, they should always be the last parameter (unless they can't be, e.g. due to a params array), named "cancellationToken", and be non-nullable. - Ensure they're forwarded appropriately from one API to the next. There may be some more missing cases, but I fixed all the ones I found. - Make the XML docs consistent. - Remove unnecessary cancellation-related analyzer suppressions
### Motivation and Context Clean up exposure and handling of CancellationToken in public APIs. ### Description - In public API, they should always be the last parameter (unless they can't be, e.g. due to a params array), named "cancellationToken", and be non-nullable. - Ensure they're forwarded appropriately from one API to the next. There may be some more missing cases, but I fixed all the ones I found. - Make the XML docs consistent. - Remove unnecessary cancellation-related analyzer suppressions
### Motivation and Context Clean up exposure and handling of CancellationToken in public APIs. ### Description - In public API, they should always be the last parameter (unless they can't be, e.g. due to a params array), named "cancellationToken", and be non-nullable. - Ensure they're forwarded appropriately from one API to the next. There may be some more missing cases, but I fixed all the ones I found. - Make the XML docs consistent. - Remove unnecessary cancellation-related analyzer suppressions
Motivation and Context
Clean up exposure and handling of CancellationToken in public APIs.
Description
One issue I noticed I haven't addressed here:
Contribution Checklist
dotnet format