Consider adding default for CancellationToken parameters in interfaces / classes in 4.0.0
Closed this issue · 10 comments
It is a common pattern for interface and class methods with CancellationToken parameters to specify a default for the parameter so that it can be called without the parameter by consumers. This includes interface and class methods with CancellationToken parameters in various Microsoft libraries such as (but not limited to) the dotnet runtime and AspNetCore as well as in other libraries. For consistency with this established pattern, consider adding this to the interface and class methods where CancellationToken parameters were added in the 4.0.0 preview.
As an example:
IAsyncEnumerable GetFeatureNamesAsync(CancellationToken cancellationToken = default);
Agreed. This is an oversight. Thanks @brental.
@zhiyuanliang-ms @rossgrambo can one you take this on. Please check all interfaces/types applicable.
@zhenlan Jimmy, Ross and I had a discussion about this issue. I just shared the teams chat history to you.
@zhenlan @brental
Not providing default value for cancellationToken is by design. Here is the discussion of this API: #250 (comment)
Last month, when we saw @brental opened this issue, we had another discussion about it.
We agree that it will make the caller code look simpler if there is a default value for cancellation token.
But, omitting cancellation token on async method invocation is not a good practice. Caller should be explicit that it does't intend to be canceled. It is terrible to accidentally omit the propagation of cancellation.
If .NET has established the paradigm, we should not go against the it. So we did some investigation about how common the default value pattern is.
Cancellation token with default value is not common in the Azure .NET SDK.
: https://github.com/search?q=repo%3AAzure%2Fazure-sdk-for-net+%22%2C+CancellationToken+token%22&type=code
System.IO lib provides default value for cancellation token: https://learn.microsoft.com/en-us/dotnet/api/system.io.stream.readasync?view=net-8.0&source=docs
System.Net.Http doesn't provide default value for cancellation token: https://learn.microsoft.com/en-us/dotnet/api/system.net.http.httpclient.postasync?view=net-8.0#system-net-http-httpclient-postasync(system-string-system-net-http-httpcontent-system-threading-cancellationtoken)
System.Data.Entity doesn't provide default value for cancellation token: https://learn.microsoft.com/en-us/dotnet/api/system.data.entity.queryableextensions.anyasync?view=entity-framework-6.2.0#system-data-entity-queryableextensions-anyasync-1(system-linq-iqueryable((-0))-system-threading-cancellationtoken)
According to our investigation, it seems like there is no such paradigm. So we decided to keep the current design.
This is a code snippet from System.Net.Http:
You can see that there are two APIs: 1. PostAsync(String, HttpContent, CancellationToken) and 2. PostAsync(String, HttpContent)
PostAsync(String, HttpContent, CancellationToken) will be called by PostAsync(String, HttpContent) and CancellationToken.None will be used.
This is how we implemented FeatureManager.IsEnabledAsync:
Maybe, we can update it to
public async Task<bool> IsEnabledAsync(string feature)
{
return await IsEnabledAsync(feature, CancellationToken.None);
}The point about the caller being explicit that it doesn't want to be cancelled does makes sense.
However, the investigation done into whether the default is being used in .NET seems like it has missed a lot cases where it is used.
For example the search in the Azure SDK links to the following search:
https://github.com/search?q=repo%3AAzure%2Fazure-sdk-for-net+%22%2C+CancellationToken+token%22&type=code
That search omits cases where the Cancellationtoken parameter is named cancellationToken rather than token and potentially cases where the token is the first parameter. These searches show more cases of using default:
https://github.com/search?q=repo%3AAzure%2Fazure-sdk-for-net+%22cancellationToken+token+%3D+default%22&type=code
These searches do show cases where default is not being used:
https://github.com/search?q=repo%3AAzure%2Fazure-sdk-for-net+%22cancellationToken+token%2C%22&type=code
So, there is definitely a mix of it being used and not being used, but seems like there are more cases where it is being used in the Azure SDK.
However, doing similar searches in the dotnet runtime and aspnetcore repos does show a mix as well, but in those two repos it is the opposite and there are more cases without the default then with.
So, given that there is a mix of default and no default it seems like the point about the caller being explicit that it doesn't want to be cancelled probably gives a good enough reason not to have the default. Consumers who do want that overload could always implement extension methods without the parameter if they really wanted it.
However, I would also point out that looking at the comparison of the System.Net.Http methods and the FeatureManager.IsEnabledAsync methods it seems like the point was that they are equivalent. However, the two FeatureManager.IsEnabledAsync methods shown do not accept a Cancellation token at all where as the System.Net.Http methods do. So, I am not sure what that comparison was meant to highlight? It seems like the two FeatureManager.IsEnabledAsync should have a CancellationToken parameter though. Is it going to be added to those as well?
@brental Thank you for your continued interest and support for this repo. Appreciate you taking the time to do the investigation.
That search omits cases where the Cancellationtoken parameter is named cancellationToken rather than token and potentially cases where the token is the first parameter.
Sorry, I was wrong.
So, there is definitely a mix of it being used and not being used, but seems like there are more cases where it is being used in the Azure SDK.
Then, it makes the option to provide default value more appealing.
However, I would also point out that looking at the comparison of the System.Net.Http methods and the FeatureManager.IsEnabledAsync methods it seems like the point was that they are equivalent. However, the two FeatureManager.IsEnabledAsync methods shown do not accept a Cancellation token at all where as the System.Net.Http methods do. So, I am not sure what that comparison was meant to highlight? It seems like the two FeatureManager.IsEnabledAsync should have a CancellationToken parameter though. Is it going to be added to those as well?
I've updated my comment. Sorry for the unclearness.
No worries @zhiyuanliang-ms.
Then, it makes the option to provide default value more appealing.
My initial thoughts were that the majority of the code in the dotnet runtime and aspnetcore libraries had the default for CancellationToken parameters. Those searches I did earlier show that is not the case. They also show that the Azure SDK has more usages of default but it is still a mix of using the default and not using the default. So, I would be happy if you end up sticking with the decision not to use the default it in this library. The reasoning for not using it (the caller being explicit) makes sense to me.
It sounds like there might still be a discussion required with @zhenlan but I can close this issue without any changes if the outcome is that default will not be used.
There is still the issue of Feature manager methods not having Cancellation token parameters, but a separate issue could be raised for that.
It's good practice to make CancellationToken required for nonpublic APIs. It ensures the proper propagation of the CancellationToken between method calls. But for public APIs, IMO, we should give people an option.
@zhiyuanliang-ms wasn't the outcome of any previous discussion on this that .NET IO classes included default cancellation so it would reasonable for us to do the same?
@jimmyca15 Sorry for the misunderstanding. PR #395 sent out.
#395 has been merged.