feat!: add CancellationTokens, ValueTasks hooks
This PR is a combination of https://github.com/open-feature/dotnet-sdk/pull/184 and https://github.com/open-feature/dotnet-sdk/pull/185. Changes include:
- adding cancellation tokens
- in all cases where async operations include side-effects (
setProviderAsync,InitializeAsync, I've specified in the in-line doc that the cancellation token's purpose is to cancel such side-effects - so setting a provider and canceling that operation still results in that provider's being set, but async side-effect should be cancelled. I'm interested in feedback here, I think we need to consider the semantics around this... I suppose the alternative would be to always ensure any state changes only occur after async side-effects, if they weren't cancelled beforehand.
- in all cases where async operations include side-effects (
- adding "Async" suffix to all async methods
- remove deprecated sync
SetProvidermethods - Using
ValueTaskfor hook methods- I've decided against converting all
TaskstoValueTasks, from the official .NET docs:
the default choice for any asynchronous method that does not return a result should be to return a Task. Only if performance analysis proves it worthwhile should a ValueTask be used instead of a Task.
- I think for hooks,
ValueTaskespecially makes sense since often hooks are synchronous, in fact async hooks are probably the less likely variant. - I've kept the resolver methods as
Task, but there could be an argument for making themValueTask, since some providers resolve asynchronously. - I'm still a bit dubious on the entire idea of
ValueTask, so I'm really interested in feedback here
- I've decided against converting all
- associated test updates
UPDATE:
After chewing on this for a night, I'm starting to feel:
- We should simply remove cancellation tokens from Init/Shutdown. We can always add them later, which would be non-breaking. I think the value is low and the complexity is potentially high.
- ValueTask is only a good idea for hooks, because:
- Hooks will very often be synchronous under the hood
- We (SDK authors) await the hooks, not consumer code, so we can be careful of the potential pitfalls of ValueTask. I think everywhere else we should stick to Task.
After chewing on this for a night, I'm starting to feel:
- We should simply remove cancellation tokens from Init/Shutdown. We can always add them later, which would be non-breaking. I think the value is low and the complexity is potentially high.
- ValueTask is only a good idea for hooks, because:
- Hooks will very often be synchronous under the hood
- We (SDK authors) await the hooks, not consumer code, so we can be careful of the potential pitfalls of ValueTask. I think everywhere else we should stick to Task.
I think that we should probably have tests which have a dummy async provider that blocks evaluations until cancelled, then we should start an evaluation and cancel it to make sure all the steps happen. Maybe something similar with a hook implementation.
That's a good idea. Will add.
I think that we should probably have tests which have a dummy async provider that blocks evaluations until cancelled, then we should start an evaluation and cancel it to make sure all the steps happen. Maybe something similar with a hook implementation.
That's a good idea. Will add.
I've added tests for cancellation (evaluation/resolution and hooks): https://github.com/open-feature/dotnet-sdk/pull/268/commits/036d7a45c5627c4fb17ce7ab7208cd52ca19b7ca