Skip to content

feat!: Thread safe hooks, provider, and context #79

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 13 commits into from
Oct 12, 2022
Merged

feat!: Thread safe hooks, provider, and context #79

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
9e4ee6d
Immutable hooks.
kinyoklion Oct 7, 2022
08e68ba
Make references volatile, always use the global provider instead of t…
kinyoklion Oct 7, 2022
bd7ba04
Reverse hooks. Make hooks in FeatureClient a ConcurrentStack.
kinyoklion Oct 7, 2022
dda0f28
Update documentation on GetHooks methods.
kinyoklion Oct 7, 2022
a86fb83
Use the same provider reference for the duration of an evaluation.
kinyoklion Oct 10, 2022
4a6736f
Use slim read/write lock.
kinyoklion Oct 10, 2022
392a005
Update to use try/finally.
kinyoklion Oct 10, 2022
b75e3e3
Add documentation.
kinyoklion Oct 10, 2022
c7f2cc3
Use no-op provider when global provider is null.
kinyoklion Oct 10, 2022
f70d391
Add documentation on adding hooks.
kinyoklion Oct 11, 2022
51cf8f2
Add locking for client context.
kinyoklion Oct 11, 2022
b6658b6
Use normal locks for the client level.
kinyoklion Oct 11, 2022
094f3d2
Add comment about disposing.
kinyoklion Oct 11, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 3 additions & 4 deletions src/OpenFeatureSDK/FeatureProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
using System;
using System.Collections.Generic;
using System.Collections.Immutable;
using System.Threading.Tasks;
using OpenFeatureSDK.Model;

Expand All @@ -22,8 +21,8 @@ public abstract class FeatureProvider
/// error (if applicable): Provider, Invocation, Client, API
/// finally: Provider, Invocation, Client, API
/// </summary>
/// <returns></returns>
public virtual IReadOnlyList<Hook> GetProviderHooks() => Array.Empty<Hook>();
/// <returns>Immutable list of hooks</returns>
public virtual IImmutableList<Hook> GetProviderHooks() => ImmutableList<Hook>.Empty;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternatively this could be enumerable like other places, with documentation that the provider must use a thread-safe implementation. Technically a readonlylist, with the same constraint, would also works. I think immutable directs people in a way that the easiest solution is also the correct solution.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think immutable directs people in a way that the easiest solution is also the correct solution.

Agreed.


/// <summary>
/// Metadata describing the provider.
Expand Down
20 changes: 10 additions & 10 deletions src/OpenFeatureSDK/Model/FlagEvaluationOptions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
using System.Collections.Generic;
using System.Collections.Immutable;

namespace OpenFeatureSDK.Model
{
Expand All @@ -12,33 +12,33 @@ public class FlagEvaluationOptions
/// <summary>
/// A immutable list of <see cref="Hook"/>
/// </summary>
public IReadOnlyList<Hook> Hooks { get; }
public IImmutableList<Hook> Hooks { get; }

/// <summary>
/// A immutable dictionary of hook hints
/// </summary>
public IReadOnlyDictionary<string, object> HookHints { get; }
public IImmutableDictionary<string, object> HookHints { get; }

/// <summary>
/// Initializes a new instance of the <see cref="FlagEvaluationOptions"/> class.
/// </summary>
/// <param name="hooks"></param>
/// <param name="hooks">An immutable list of hooks to use during evaluation</param>
/// <param name="hookHints">Optional - a list of hints that are passed through the hook lifecycle</param>
public FlagEvaluationOptions(IReadOnlyList<Hook> hooks, IReadOnlyDictionary<string, object> hookHints = null)
public FlagEvaluationOptions(IImmutableList<Hook> hooks, IImmutableDictionary<string, object> hookHints = null)
{
this.Hooks = hooks;
this.HookHints = hookHints ?? new Dictionary<string, object>();
this.HookHints = hookHints ?? ImmutableDictionary<string, object>.Empty;
}

/// <summary>
/// Initializes a new instance of the <see cref="FlagEvaluationOptions"/> class.
/// </summary>
/// <param name="hook"></param>
/// <param name="hook">A hook to use during the evaluation</param>
/// <param name="hookHints">Optional - a list of hints that are passed through the hook lifecycle</param>
public FlagEvaluationOptions(Hook hook, IReadOnlyDictionary<string, object> hookHints = null)
public FlagEvaluationOptions(Hook hook, ImmutableDictionary<string, object> hookHints = null)
{
this.Hooks = new[] { hook };
this.HookHints = hookHints ?? new Dictionary<string, object>();
this.Hooks = ImmutableList.Create(hook);
this.HookHints = hookHints ?? ImmutableDictionary<string, object>.Empty;
}
}
}
86 changes: 71 additions & 15 deletions src/OpenFeatureSDK/OpenFeature.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
using System.Collections.Concurrent;
using System.Collections.Generic;
using System.Linq;
using System.Threading;
using Microsoft.Extensions.Logging;
using OpenFeatureSDK.Model;

Expand All @@ -13,7 +16,10 @@ public sealed class OpenFeature
{
private EvaluationContext _evaluationContext = EvaluationContext.Empty;
private FeatureProvider _featureProvider = new NoOpFeatureProvider();
private readonly List<Hook> _hooks = new List<Hook>();
private readonly ConcurrentStack<Hook> _hooks = new ConcurrentStack<Hook>();

private readonly ReaderWriterLockSlim _evaluationContextLock = new ReaderWriterLockSlim();
private readonly ReaderWriterLockSlim _featureProviderLock = new ReaderWriterLockSlim();

/// <summary>
/// Singleton instance of OpenFeature
Expand All @@ -30,19 +36,41 @@ private OpenFeature() { }
/// Sets the feature provider
/// </summary>
/// <param name="featureProvider">Implementation of <see cref="FeatureProvider"/></param>
public void SetProvider(FeatureProvider featureProvider) => this._featureProvider = featureProvider;
public void SetProvider(FeatureProvider featureProvider)
{
this._featureProviderLock.EnterWriteLock();
try
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

None of these code blocks can throw, but when using this type of lock I think using try/finally is a customary way to release the lock.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, agreed. In fact, I made a little lock-wrapper class that allows us to use a try-with-resources block in Java (I think c# has an equivalent with the using keyword and IDisposable implementations).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it could be better to make a couple little RAII objects for locks.

I am not sure for a couple reasons. The first is that it would generate an extra bit of garbage for each access to these fields. It would likely be negligible.

The second is that all the locking is currently only exposed in this one file.

If we were to need to expose the locking outside this file, then I could see the idea of returning a read lock protected object which the consumer could use in a using block.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Especially for double locks, where they could nest the using statement.

{
this._featureProvider = featureProvider;
}
finally
{
this._featureProviderLock.ExitWriteLock();
}
}

/// <summary>
/// Gets the feature provider
/// </summary>
/// <returns><see cref="FeatureProvider"/></returns>
public FeatureProvider GetProvider() => this._featureProvider;
public FeatureProvider GetProvider()
{
this._featureProviderLock.EnterReadLock();
try
{
return this._featureProvider;
}
finally
{
this._featureProviderLock.ExitReadLock();
}
}

/// <summary>
/// Gets providers metadata
/// </summary>
/// <returns><see cref="ClientMetadata"/></returns>
public Metadata GetProviderMetadata() => this._featureProvider.GetMetadata();
public Metadata GetProviderMetadata() => this.GetProvider().GetMetadata();

/// <summary>
/// Create a new instance of <see cref="FeatureClient"/> using the current provider
Expand All @@ -52,26 +80,32 @@ private OpenFeature() { }
/// <param name="logger">Logger instance used by client</param>
/// <param name="context">Context given to this client</param>
/// <returns><see cref="FeatureClient"/></returns>
public FeatureClient GetClient(string name = null, string version = null, ILogger logger = null, EvaluationContext context = null) =>
new FeatureClient(this._featureProvider, name, version, logger, context);
public FeatureClient GetClient(string name = null, string version = null, ILogger logger = null,
EvaluationContext context = null) =>
new FeatureClient(name, version, logger, context);

/// <summary>
/// Appends list of hooks to global hooks list
/// </summary>
/// <param name="hooks">A list of <see cref="Hook"/></param>
public void AddHooks(IEnumerable<Hook> hooks) => this._hooks.AddRange(hooks);
public void AddHooks(IEnumerable<Hook> hooks) => this._hooks.PushRange(hooks.ToArray());

/// <summary>
/// Adds a hook to global hooks list
/// </summary>
/// <param name="hook">A list of <see cref="Hook"/></param>
public void AddHooks(Hook hook) => this._hooks.Add(hook);
public void AddHooks(Hook hook) => this._hooks.Push(hook);

/// <summary>
/// Returns the global immutable hooks list
/// Enumerates the global hooks.
/// <para>
/// The items enumerated will reflect the registered hooks
/// at the start of enumeration. Hooks added during enumeration
/// will not be included.
/// </para>
/// </summary>
/// <returns>A immutable list of <see cref="Hook"/></returns>
public IReadOnlyList<Hook> GetHooks() => this._hooks.AsReadOnly();
/// <returns>Enumeration of <see cref="Hook"/></returns>
public IEnumerable<Hook> GetHooks() => this._hooks.Reverse();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the open feature API and Client, the SDK controls the implementation type and can promise the use of a concurrent collection.

In the provider this is not the case, so that is constrained to an immutable collection.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I am reversing these for now. I wasn't completely comfortable with all the test changes required for them to be in the opposite order.


/// <summary>
/// Removes all hooks from global hooks list
Expand All @@ -81,13 +115,35 @@ public FeatureClient GetClient(string name = null, string version = null, ILogge
/// <summary>
/// Sets the global <see cref="EvaluationContext"/>
/// </summary>
/// <param name="context"></param>
public void SetContext(EvaluationContext context) => this._evaluationContext = context ?? EvaluationContext.Empty;
/// <param name="context">The <see cref="EvaluationContext"/> to set</param>
public void SetContext(EvaluationContext context)
{
this._evaluationContextLock.EnterWriteLock();
try
{
this._evaluationContext = context ?? EvaluationContext.Empty;
}
finally
{
this._evaluationContextLock.ExitWriteLock();
}
}

/// <summary>
/// Gets the global <see cref="EvaluationContext"/>
/// </summary>
/// <returns></returns>
public EvaluationContext GetContext() => this._evaluationContext;
/// <returns>An <see cref="EvaluationContext"/></returns>
public EvaluationContext GetContext()
{
this._evaluationContextLock.EnterReadLock();
try
{
return this._evaluationContext;
}
finally
{
this._evaluationContextLock.ExitReadLock();
}
}
}
}
70 changes: 51 additions & 19 deletions src/OpenFeatureSDK/OpenFeatureClient.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
using System;
using System.Collections.Concurrent;
using System.Collections.Generic;
using System.Linq;
using System.Threading.Tasks;
Expand All @@ -17,11 +18,32 @@ namespace OpenFeatureSDK
public sealed class FeatureClient : IFeatureClient
{
private readonly ClientMetadata _metadata;
private readonly FeatureProvider _featureProvider;
private readonly List<Hook> _hooks = new List<Hook>();
private readonly ConcurrentStack<Hook> _hooks = new ConcurrentStack<Hook>();
private readonly ILogger _logger;
private EvaluationContext _evaluationContext;

/// <summary>
/// Get a provider and an associated typed flag resolution method.
/// <para>
/// The global provider could change between two accesses, so in order to safely get provider information we
/// must first alias it and then use that alias to access everything we need.
/// </para>
/// </summary>
/// <param name="method">
/// This method should return the desired flag resolution method from the given provider reference.
/// </param>
/// <typeparam name="T">The type of the resolution method</typeparam>
/// <returns>A tuple containing a resolution method and the provider it came from.</returns>
private (Func<string, T, EvaluationContext, Task<ResolutionDetails<T>>>, FeatureProvider)
ExtractProvider<T>(
Func<FeatureProvider, Func<string, T, EvaluationContext, Task<ResolutionDetails<T>>>> method)
{
// Alias the provider reference so getting the method and returning the provider are
// guaranteed to be the same object.
var provider = OpenFeature.Instance.GetProvider();
return (method(provider), provider);
}

/// <summary>
/// Gets the EvaluationContext of this client<see cref="EvaluationContext"/>
/// </summary>
Expand All @@ -36,15 +58,13 @@ public sealed class FeatureClient : IFeatureClient
/// <summary>
/// Initializes a new instance of the <see cref="FeatureClient"/> class.
/// </summary>
/// <param name="featureProvider">Feature provider used by client <see cref="FeatureProvider"/></param>
/// <param name="name">Name of client <see cref="ClientMetadata"/></param>
/// <param name="version">Version of client <see cref="ClientMetadata"/></param>
/// <param name="logger">Logger used by client</param>
/// <param name="context">Context given to this client</param>
/// <exception cref="ArgumentNullException">Throws if any of the required parameters are null</exception>
public FeatureClient(FeatureProvider featureProvider, string name, string version, ILogger logger = null, EvaluationContext context = null)
Copy link
Member Author

@kinyoklion kinyoklion Oct 10, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just realized I need to re-introduce the equivalent of this check elsewhere.

I lifted the behavior the Java SDK has. Which is to log that it was not set, and then use the NoOp provider.

public FeatureClient(string name, string version, ILogger logger = null, EvaluationContext context = null)
{
this._featureProvider = featureProvider ?? throw new ArgumentNullException(nameof(featureProvider));
this._metadata = new ClientMetadata(name, version);
this._logger = logger ?? new Logger<OpenFeature>(new NullLoggerFactory());
this._evaluationContext = context ?? EvaluationContext.Empty;
Expand All @@ -60,19 +80,24 @@ public FeatureClient(FeatureProvider featureProvider, string name, string versio
/// Add hook to client
/// </summary>
/// <param name="hook">Hook that implements the <see cref="Hook"/> interface</param>
public void AddHooks(Hook hook) => this._hooks.Add(hook);
public void AddHooks(Hook hook) => this._hooks.Push(hook);

/// <summary>
/// Appends hooks to client
/// </summary>
/// <param name="hooks">A list of Hooks that implement the <see cref="Hook"/> interface</param>
public void AddHooks(IEnumerable<Hook> hooks) => this._hooks.AddRange(hooks);
public void AddHooks(IEnumerable<Hook> hooks) => this._hooks.PushRange(hooks.ToArray());

/// <summary>
/// Return a immutable list of hooks that are registered against the client
/// Enumerates the global hooks.
/// <para>
/// The items enumerated will reflect the registered hooks
/// at the start of enumeration. Hooks added during enumeration
/// will not be included.
/// </para>
/// </summary>
/// <returns>A list of immutable hooks</returns>
public IReadOnlyList<Hook> GetHooks() => this._hooks.ToList().AsReadOnly();
/// <returns>Enumeration of <see cref="Hook"/></returns>
public IEnumerable<Hook> GetHooks() => this._hooks.Reverse();

/// <summary>
/// Removes all hooks from the client
Expand Down Expand Up @@ -101,7 +126,8 @@ public async Task<bool> GetBooleanValue(string flagKey, bool defaultValue, Evalu
/// <returns>Resolved flag details <see cref="FlagEvaluationDetails{T}"/></returns>
public async Task<FlagEvaluationDetails<bool>> GetBooleanDetails(string flagKey, bool defaultValue,
EvaluationContext context = null, FlagEvaluationOptions config = null) =>
await this.EvaluateFlag(this._featureProvider.ResolveBooleanValue, FlagValueType.Boolean, flagKey,
await this.EvaluateFlag(this.ExtractProvider<bool>(provider => provider.ResolveBooleanValue),
FlagValueType.Boolean, flagKey,
defaultValue, context, config);

/// <summary>
Expand All @@ -126,7 +152,8 @@ public async Task<string> GetStringValue(string flagKey, string defaultValue, Ev
/// <returns>Resolved flag details <see cref="FlagEvaluationDetails{T}"/></returns>
public async Task<FlagEvaluationDetails<string>> GetStringDetails(string flagKey, string defaultValue,
EvaluationContext context = null, FlagEvaluationOptions config = null) =>
await this.EvaluateFlag(this._featureProvider.ResolveStringValue, FlagValueType.String, flagKey,
await this.EvaluateFlag(this.ExtractProvider<string>(provider => provider.ResolveStringValue),
FlagValueType.String, flagKey,
defaultValue, context, config);

/// <summary>
Expand All @@ -151,7 +178,8 @@ public async Task<int> GetIntegerValue(string flagKey, int defaultValue, Evaluat
/// <returns>Resolved flag details <see cref="FlagEvaluationDetails{T}"/></returns>
public async Task<FlagEvaluationDetails<int>> GetIntegerDetails(string flagKey, int defaultValue,
EvaluationContext context = null, FlagEvaluationOptions config = null) =>
await this.EvaluateFlag(this._featureProvider.ResolveIntegerValue, FlagValueType.Number, flagKey,
await this.EvaluateFlag(this.ExtractProvider<int>(provider => provider.ResolveIntegerValue),
FlagValueType.Number, flagKey,
defaultValue, context, config);

/// <summary>
Expand All @@ -177,7 +205,8 @@ public async Task<double> GetDoubleValue(string flagKey, double defaultValue,
/// <returns>Resolved flag details <see cref="FlagEvaluationDetails{T}"/></returns>
public async Task<FlagEvaluationDetails<double>> GetDoubleDetails(string flagKey, double defaultValue,
EvaluationContext context = null, FlagEvaluationOptions config = null) =>
await this.EvaluateFlag(this._featureProvider.ResolveDoubleValue, FlagValueType.Number, flagKey,
await this.EvaluateFlag(this.ExtractProvider<double>(provider => provider.ResolveDoubleValue),
FlagValueType.Number, flagKey,
defaultValue, context, config);

/// <summary>
Expand All @@ -202,14 +231,17 @@ public async Task<Value> GetObjectValue(string flagKey, Value defaultValue, Eval
/// <returns>Resolved flag details <see cref="FlagEvaluationDetails{T}"/></returns>
public async Task<FlagEvaluationDetails<Value>> GetObjectDetails(string flagKey, Value defaultValue,
EvaluationContext context = null, FlagEvaluationOptions config = null) =>
await this.EvaluateFlag(this._featureProvider.ResolveStructureValue, FlagValueType.Object, flagKey,
await this.EvaluateFlag(this.ExtractProvider<Value>(provider => provider.ResolveStructureValue),
FlagValueType.Object, flagKey,
defaultValue, context, config);

private async Task<FlagEvaluationDetails<T>> EvaluateFlag<T>(
Func<string, T, EvaluationContext, Task<ResolutionDetails<T>>> resolveValueDelegate,
(Func<string, T, EvaluationContext, Task<ResolutionDetails<T>>>, FeatureProvider) providerInfo,
FlagValueType flagValueType, string flagKey, T defaultValue, EvaluationContext context = null,
FlagEvaluationOptions options = null)
{
var resolveValueDelegate = providerInfo.Item1;
var provider = providerInfo.Item2;
// New up a evaluation context if one was not provided.
if (context == null)
{
Expand All @@ -225,9 +257,9 @@ private async Task<FlagEvaluationDetails<T>> EvaluateFlag<T>(

var allHooks = new List<Hook>()
.Concat(OpenFeature.Instance.GetHooks())
.Concat(this._hooks)
.Concat(this.GetHooks())
.Concat(options?.Hooks ?? Enumerable.Empty<Hook>())
.Concat(this._featureProvider.GetProviderHooks())
.Concat(provider.GetProviderHooks())
.ToList()
.AsReadOnly();

Expand All @@ -241,7 +273,7 @@ private async Task<FlagEvaluationDetails<T>> EvaluateFlag<T>(
flagKey,
defaultValue,
flagValueType, this._metadata,
OpenFeature.Instance.GetProviderMetadata(),
provider.GetMetadata(),
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't want to call GetProviderMetadata here on the OpenFeature singleton, because it could have change between when we got the resolution method and here. We want to be sure to only use the cached reference.

evaluationContextBuilder.Build()
);

Expand Down
Loading