dotnet-sdk icon indicating copy to clipboard operation
dotnet-sdk copied to clipboard

feat!: add CancellationTokens, ValueTasks hooks

Open toddbaert opened this issue 1 year ago • 3 comments

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.
  • adding "Async" suffix to all async methods
  • remove deprecated sync SetProvider methods
  • Using ValueTask for hook methods

    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, ValueTask especially 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 them ValueTask, 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
  • 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.

toddbaert avatar May 01 '24 18:05 toddbaert

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.

toddbaert avatar May 02 '24 13:05 toddbaert

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.

toddbaert avatar May 06 '24 12:05 toddbaert

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

toddbaert avatar May 13 '24 18:05 toddbaert