Skip to content

Commit

Permalink
feat!: add CancellationTokens, ValueTasks hooks (#268)
Browse files Browse the repository at this point in the history
This PR is a combination of
#184 and
#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
- I've decided against converting all `Tasks` to `ValueTasks`, from the
[official .NET
docs](https://learn.microsoft.com/en-us/dotnet/api/system.threading.tasks.valuetask?view=net-8.0):
> the default choice for any asynchronous method that does not return a
result should be to return a
[Task](https://learn.microsoft.com/en-us/dotnet/api/system.threading.tasks.task?view=net-8.0).
Only if performance analysis proves it worthwhile should a ValueTask be
used instead of a
[Task](https://learn.microsoft.com/en-us/dotnet/api/system.threading.tasks.task?view=net-8.0).
- 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.

---------

Signed-off-by: Austin Drenski <austin@austindrenski.io>
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
Co-authored-by: Austin Drenski <austin@austindrenski.io>
Co-authored-by: André Silva <2493377+askpt@users.noreply.github.com>
Co-authored-by: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com>
  • Loading branch information
4 people committed Jun 17, 2024
1 parent acd0385 commit 33154d2
Show file tree
Hide file tree
Showing 21 changed files with 1,001 additions and 940 deletions.
640 changes: 320 additions & 320 deletions README.md

Large diffs are not rendered by default.

33 changes: 5 additions & 28 deletions src/OpenFeature/Api.cs
Original file line number Diff line number Diff line change
Expand Up @@ -37,40 +37,17 @@ static Api() { }
private Api() { }

/// <summary>
/// Sets the default feature provider to given clientName without awaiting its initialization.
/// </summary>
/// <remarks>The provider cannot be set to null. Attempting to set the provider to null has no effect.</remarks>
/// <param name="featureProvider">Implementation of <see cref="FeatureProvider"/></param>
[Obsolete("Will be removed in later versions; use SetProviderAsync, which can be awaited")]
public void SetProvider(FeatureProvider featureProvider)
{
this._eventExecutor.RegisterDefaultFeatureProvider(featureProvider);
_ = this._repository.SetProvider(featureProvider, this.GetContext());
}

/// <summary>
/// Sets the default feature provider. In order to wait for the provider to be set, and initialization to complete,
/// Sets the feature provider. In order to wait for the provider to be set, and initialization to complete,
/// await the returned task.
/// </summary>
/// <remarks>The provider cannot be set to null. Attempting to set the provider to null has no effect.</remarks>
/// <param name="featureProvider">Implementation of <see cref="FeatureProvider"/></param>
public async Task SetProviderAsync(FeatureProvider? featureProvider)
public async Task SetProviderAsync(FeatureProvider featureProvider)
{
this._eventExecutor.RegisterDefaultFeatureProvider(featureProvider);
await this._repository.SetProvider(featureProvider, this.GetContext()).ConfigureAwait(false);
await this._repository.SetProviderAsync(featureProvider, this.GetContext()).ConfigureAwait(false);
}

/// <summary>
/// Sets the feature provider to given clientName without awaiting its initialization.
/// </summary>
/// <param name="clientName">Name of client</param>
/// <param name="featureProvider">Implementation of <see cref="FeatureProvider"/></param>
[Obsolete("Will be removed in later versions; use SetProviderAsync, which can be awaited")]
public void SetProvider(string clientName, FeatureProvider featureProvider)
{
this._eventExecutor.RegisterClientFeatureProvider(clientName, featureProvider);
_ = this._repository.SetProvider(clientName, featureProvider, this.GetContext());
}

/// <summary>
/// Sets the feature provider to given clientName. In order to wait for the provider to be set, and
Expand All @@ -85,7 +62,7 @@ public async Task SetProviderAsync(string clientName, FeatureProvider featurePro
throw new ArgumentNullException(nameof(clientName));
}
this._eventExecutor.RegisterClientFeatureProvider(clientName, featureProvider);
await this._repository.SetProvider(clientName, featureProvider, this.GetContext()).ConfigureAwait(false);
await this._repository.SetProviderAsync(clientName, featureProvider, this.GetContext()).ConfigureAwait(false);
}

/// <summary>
Expand Down Expand Up @@ -248,7 +225,7 @@ public EvaluationContext GetContext()
/// Once shut down is complete, API is reset and ready to use again.
/// </para>
/// </summary>
public async Task Shutdown()
public async Task ShutdownAsync()
{
await using (this._eventExecutor.ConfigureAwait(false))
await using (this._repository.ConfigureAwait(false))
Expand Down
7 changes: 4 additions & 3 deletions src/OpenFeature/EventExecutor.cs
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@

namespace OpenFeature
{
internal delegate Task ShutdownDelegate(CancellationToken cancellationToken);

internal sealed partial class EventExecutor : IAsyncDisposable
{
private readonly object _lockObj = new object();
Expand All @@ -30,7 +32,7 @@ public EventExecutor()
eventProcessing.Start();
}

public ValueTask DisposeAsync() => new(this.Shutdown());
public ValueTask DisposeAsync() => new(this.ShutdownAsync());

internal void SetLogger(ILogger logger) => this._logger = logger;

Expand Down Expand Up @@ -317,10 +319,9 @@ private void InvokeEventHandler(EventHandlerDelegate eventHandler, Event e)
}
}

public async Task Shutdown()
public async Task ShutdownAsync()
{
this.EventChannel.Writer.Complete();

await this.EventChannel.Reader.Completion.ConfigureAwait(false);
}

Expand Down
34 changes: 21 additions & 13 deletions src/OpenFeature/FeatureProvider.cs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
using System.Collections.Immutable;
using System.Threading;
using System.Threading.Channels;
using System.Threading.Tasks;
using OpenFeature.Constant;
Expand Down Expand Up @@ -43,49 +44,54 @@ public abstract class FeatureProvider
/// <param name="flagKey">Feature flag key</param>
/// <param name="defaultValue">Default value</param>
/// <param name="context"><see cref="EvaluationContext"/></param>
/// <param name="cancellationToken">The <see cref="CancellationToken"/>.</param>
/// <returns><see cref="ResolutionDetails{T}"/></returns>
public abstract Task<ResolutionDetails<bool>> ResolveBooleanValue(string flagKey, bool defaultValue,
EvaluationContext? context = null);
public abstract Task<ResolutionDetails<bool>> ResolveBooleanValueAsync(string flagKey, bool defaultValue,
EvaluationContext? context = null, CancellationToken cancellationToken = default);

/// <summary>
/// Resolves a string feature flag
/// </summary>
/// <param name="flagKey">Feature flag key</param>
/// <param name="defaultValue">Default value</param>
/// <param name="context"><see cref="EvaluationContext"/></param>
/// <param name="cancellationToken">The <see cref="CancellationToken"/>.</param>
/// <returns><see cref="ResolutionDetails{T}"/></returns>
public abstract Task<ResolutionDetails<string>> ResolveStringValue(string flagKey, string defaultValue,
EvaluationContext? context = null);
public abstract Task<ResolutionDetails<string>> ResolveStringValueAsync(string flagKey, string defaultValue,
EvaluationContext? context = null, CancellationToken cancellationToken = default);

/// <summary>
/// Resolves a integer feature flag
/// </summary>
/// <param name="flagKey">Feature flag key</param>
/// <param name="defaultValue">Default value</param>
/// <param name="context"><see cref="EvaluationContext"/></param>
/// <param name="cancellationToken">The <see cref="CancellationToken"/>.</param>
/// <returns><see cref="ResolutionDetails{T}"/></returns>
public abstract Task<ResolutionDetails<int>> ResolveIntegerValue(string flagKey, int defaultValue,
EvaluationContext? context = null);
public abstract Task<ResolutionDetails<int>> ResolveIntegerValueAsync(string flagKey, int defaultValue,
EvaluationContext? context = null, CancellationToken cancellationToken = default);

/// <summary>
/// Resolves a double feature flag
/// </summary>
/// <param name="flagKey">Feature flag key</param>
/// <param name="defaultValue">Default value</param>
/// <param name="context"><see cref="EvaluationContext"/></param>
/// <param name="cancellationToken">The <see cref="CancellationToken"/>.</param>
/// <returns><see cref="ResolutionDetails{T}"/></returns>
public abstract Task<ResolutionDetails<double>> ResolveDoubleValue(string flagKey, double defaultValue,
EvaluationContext? context = null);
public abstract Task<ResolutionDetails<double>> ResolveDoubleValueAsync(string flagKey, double defaultValue,
EvaluationContext? context = null, CancellationToken cancellationToken = default);

/// <summary>
/// Resolves a structured feature flag
/// </summary>
/// <param name="flagKey">Feature flag key</param>
/// <param name="defaultValue">Default value</param>
/// <param name="context"><see cref="EvaluationContext"/></param>
/// <param name="cancellationToken">The <see cref="CancellationToken"/>.</param>
/// <returns><see cref="ResolutionDetails{T}"/></returns>
public abstract Task<ResolutionDetails<Value>> ResolveStructureValue(string flagKey, Value defaultValue,
EvaluationContext? context = null);
public abstract Task<ResolutionDetails<Value>> ResolveStructureValueAsync(string flagKey, Value defaultValue,
EvaluationContext? context = null, CancellationToken cancellationToken = default);

/// <summary>
/// Get the status of the provider.
Expand All @@ -95,7 +101,7 @@ public abstract Task<ResolutionDetails<Value>> ResolveStructureValue(string flag
/// If a provider does not override this method, then its status will be assumed to be
/// <see cref="ProviderStatus.Ready"/>. If a provider implements this method, and supports initialization,
/// then it should start in the <see cref="ProviderStatus.NotReady"/>status . If the status is
/// <see cref="ProviderStatus.NotReady"/>, then the Api will call the <see cref="Initialize" /> when the
/// <see cref="ProviderStatus.NotReady"/>, then the Api will call the <see cref="InitializeAsync" /> when the
/// provider is set.
/// </remarks>
public virtual ProviderStatus GetStatus() => ProviderStatus.Ready;
Expand All @@ -107,6 +113,7 @@ public abstract Task<ResolutionDetails<Value>> ResolveStructureValue(string flag
/// </para>
/// </summary>
/// <param name="context"><see cref="EvaluationContext"/></param>
/// <param name="cancellationToken">The <see cref="CancellationToken"/> to cancel any async side effects.</param>
/// <returns>A task that completes when the initialization process is complete.</returns>
/// <remarks>
/// <para>
Expand All @@ -118,7 +125,7 @@ public abstract Task<ResolutionDetails<Value>> ResolveStructureValue(string flag
/// the <see cref="GetStatus"/> method after initialization is complete.
/// </para>
/// </remarks>
public virtual Task Initialize(EvaluationContext context)
public virtual Task InitializeAsync(EvaluationContext context, CancellationToken cancellationToken = default)
{
// Intentionally left blank.
return Task.CompletedTask;
Expand All @@ -129,7 +136,8 @@ public virtual Task Initialize(EvaluationContext context)
/// Providers can overwrite this method, if they have special shutdown actions needed.
/// </summary>
/// <returns>A task that completes when the shutdown process is complete.</returns>
public virtual Task Shutdown()
/// <param name="cancellationToken">The <see cref="CancellationToken"/> to cancel any async side effects.</param>
public virtual Task ShutdownAsync(CancellationToken cancellationToken = default)
{
// Intentionally left blank.
return Task.CompletedTask;
Expand Down
27 changes: 16 additions & 11 deletions src/OpenFeature/Hook.cs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
using System;
using System.Collections.Generic;
using System.Threading;
using System.Threading.Tasks;
using OpenFeature.Model;

Expand All @@ -26,12 +27,13 @@ public abstract class Hook
/// </summary>
/// <param name="context">Provides context of innovation</param>
/// <param name="hints">Caller provided data</param>
/// <param name="cancellationToken">The <see cref="CancellationToken"/>.</param>
/// <typeparam name="T">Flag value type (bool|number|string|object)</typeparam>
/// <returns>Modified EvaluationContext that is used for the flag evaluation</returns>
public virtual Task<EvaluationContext> Before<T>(HookContext<T> context,
IReadOnlyDictionary<string, object>? hints = null)
public virtual ValueTask<EvaluationContext> BeforeAsync<T>(HookContext<T> context,
IReadOnlyDictionary<string, object>? hints = null, CancellationToken cancellationToken = default)
{
return Task.FromResult(EvaluationContext.Empty);
return new ValueTask<EvaluationContext>(EvaluationContext.Empty);
}

/// <summary>
Expand All @@ -40,11 +42,12 @@ public virtual Task<EvaluationContext> Before<T>(HookContext<T> context,
/// <param name="context">Provides context of innovation</param>
/// <param name="details">Flag evaluation information</param>
/// <param name="hints">Caller provided data</param>
/// <param name="cancellationToken">The <see cref="CancellationToken"/>.</param>
/// <typeparam name="T">Flag value type (bool|number|string|object)</typeparam>
public virtual Task After<T>(HookContext<T> context, FlagEvaluationDetails<T> details,
IReadOnlyDictionary<string, object>? hints = null)
public virtual ValueTask AfterAsync<T>(HookContext<T> context, FlagEvaluationDetails<T> details,
IReadOnlyDictionary<string, object>? hints = null, CancellationToken cancellationToken = default)
{
return Task.CompletedTask;
return new ValueTask();
}

/// <summary>
Expand All @@ -53,22 +56,24 @@ public virtual Task After<T>(HookContext<T> context, FlagEvaluationDetails<T> de
/// <param name="context">Provides context of innovation</param>
/// <param name="error">Exception representing what went wrong</param>
/// <param name="hints">Caller provided data</param>
/// <param name="cancellationToken">The <see cref="CancellationToken"/>.</param>
/// <typeparam name="T">Flag value type (bool|number|string|object)</typeparam>
public virtual Task Error<T>(HookContext<T> context, Exception error,
IReadOnlyDictionary<string, object>? hints = null)
public virtual ValueTask ErrorAsync<T>(HookContext<T> context, Exception error,
IReadOnlyDictionary<string, object>? hints = null, CancellationToken cancellationToken = default)
{
return Task.CompletedTask;
return new ValueTask();
}

/// <summary>
/// Called unconditionally after flag evaluation.
/// </summary>
/// <param name="context">Provides context of innovation</param>
/// <param name="hints">Caller provided data</param>
/// <param name="cancellationToken">The <see cref="CancellationToken"/>.</param>
/// <typeparam name="T">Flag value type (bool|number|string|object)</typeparam>
public virtual Task Finally<T>(HookContext<T> context, IReadOnlyDictionary<string, object>? hints = null)
public virtual ValueTask FinallyAsync<T>(HookContext<T> context, IReadOnlyDictionary<string, object>? hints = null, CancellationToken cancellationToken = default)
{
return Task.CompletedTask;
return new ValueTask();
}
}
}
Loading

0 comments on commit 33154d2

Please sign in to comment.