Async changes for 2.0.0

[toddbaert]

Hey OpenFeature .NET folks!

Recently there's been some proposed changes to the SDK for more idiomatic async method names, as well as cancellation token support. We now have a SetProviderAsync method, and here some changes to the flag evaluation methods and provider resolvers were pitched as well.

The proposed method names and signatures are consistent with .NET standard lib functions and popular libraries, so I'm in favor. However, I think we need to solidify semantics around the cancellation tokens in particular; for the SDK, and for providers, what does cancellation mean? I think it's clear that a cancellation would probably cause a flag evaluation to default, but I'm less sure about the implications on methods that imply some kind of state change (like SetProviderAsync or Shutdown).

My initial thought was that cancellation of any such methods should probably result in the implied end-state being reached, but cancel any asynchronous actions, for example:

• cancelling setProviderAsync would result in the provider being set, but the provider may now being in an error state (since it's initialization was cancelled)
• cancelling shutdown would still shut down the SDK and all providers, but asynchronous work (such as flushing telemetry or gracefully tearing-down connections) may have been cancelled

There may be more examples. I think we need to iron this out so we can give some direction to provider-authors and API users before we release these changes.

[austindrenski]

Disclosure up-front: I tend to think about cancellation from the perspective of an app container author, so my views may be under-nuanced w.r.t. the OpenFeature SDK's responsibilities to the OpenFeature spec, so hoping that others will chime in with counterexamples if the picture I'm painting is missing some of those considerations.

@toddbaert wrote #207 (comment):

However, I think we need to solidify semantics around the cancellation tokens in particular; for the SDK, and for providers, what does cancellation mean?

In a somewhat naive way, IMO, cancellation in an upstream library means stop, drop, and roll.

I think it's clear that a cancellation would probably cause a flag evaluation to default, but I'm less sure about the implications on methods that imply some kind of state change (like SetProviderAsync or Shutdown).

If we're in a method like Api.SetProviderAsync(...) when cancellation is requested, I would expect us to be in one of three states, either: (1) we haven't called to the provider yet, in which case we should cancellationToken.ThrowIfCanellationRequested(); (2) we set the provider and are waiting for provider.InitializeAsync(...) to return, in which case the provider should throw, we should catch, unset the provider, and rethrow; or (3) we've already passed (2) and we should unset the provider and call cancellationToken.ThrowIfCanellationRequested() as in (1).

For calls into methods like IFeatureClient.GetBooleanAsync(...), I would expect cancellation to also throw an exception, +/- returning Task.FromCanceled(...) or ValueTask.FromCanceled(...) as appropriate.

I don't think that cancellation anywhere else in the SDK should trigger FeatureProvider.ShutdownAsync(...), except for the case where we're un-initializing and un-setting a provider in Api.SetProviderAsync(...) because the passed cancellation token was triggered.

My initial thought was that cancellation of any such methods should probably result in the implied end-state being reached, but cancel any asynchronous actions, for example:

• cancelling setProviderAsync would result in the provider being set, but the provider may now being in an error state (since it's initialization was cancelled)
• cancelling shutdown would still shut down the SDK and all providers, but asynchronous work (such as flushing telemetry or gracefully tearing-down connections) may have been cancelled

I can see where you're coming from here, but I actually think this^ would be really dangerous. For example (admittedly a very contrived example...):

using var cts = CancellationTokenSource.CreateLinkedTokenSource(default(CancellationToken));

cts.Cancel();

if (await featureClient.GetBooleanAsync("should-delete-all-client-data", false, cts.Token).ConfigureAwait(false))
{
    // yay, delete all the data!
}
else
{
    // okay, fine, only delete some of that data...
}

[austindrenski]

@austindrenski wrote #207 (comment):

I can see where you're coming from here, but I actually think this^ would be really dangerous. For example (admittedly a very contrived example...):

One possible option to mitigate this would be to introduce overloads that do not take default values, e.g.:

- Task<bool> GetBooleanValueAsync(string flagKey, bool defaultValue, EvaluationContext? context = null, FlagEvaluationOptions? config = null)
+ Task<bool> GetBooleanValueAsync(string flagKey, bool defaultValue, EvaluationContext? context = null, FlagEvaluationOptions? config = null, CancellationToken cancellationToken = default)
+ Task<bool> GetBooleanValueAsync(string flagKey, EvaluationContext? context = null, FlagEvaluationOptions? config = null, CancellationToken cancellationToken = default)

Then this non-default-value overload could mean throws if cancelled and the original overload could be updated with a description to say returns defaultValue if cancelled.

I've actually already found myself reaching for this^ in real-world code where I don't want to pass a defaultValue and just want the underlying provider to throw if anything goes wrong (e.g. timeouts, network failures, unknown flag key).

I recognize that the OpenFeature spec provides a way to distinguish between these cases (e.g. GetBooleanDetailsAsync(...)), but IMO these overloads would provide better ergonomics/familiarity with other .NET libraries/usage patterns.