From 9e4ee6db041caa0a4fa7f82094811ac22c861547 Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Fri, 7 Oct 2022 14:34:13 -0700 Subject: [PATCH 01/13] Immutable hooks. Signed-off-by: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> --- src/OpenFeatureSDK/FeatureProvider.cs | 5 +-- .../Model/FlagEvaluationOptions.cs | 15 +++++---- src/OpenFeatureSDK/OpenFeature.cs | 10 +++--- .../OpenFeatureClientTests.cs | 21 ++++++------ .../OpenFeatureHookTests.cs | 33 ++++++++++--------- test/OpenFeatureSDK.Tests/OpenFeatureTests.cs | 13 ++++---- .../TestImplementations.cs | 3 +- 7 files changed, 54 insertions(+), 46 deletions(-) diff --git a/src/OpenFeatureSDK/FeatureProvider.cs b/src/OpenFeatureSDK/FeatureProvider.cs index 639363a8..ed9a79b5 100644 --- a/src/OpenFeatureSDK/FeatureProvider.cs +++ b/src/OpenFeatureSDK/FeatureProvider.cs @@ -1,5 +1,6 @@ using System; using System.Collections.Generic; +using System.Collections.Immutable; using System.Threading.Tasks; using OpenFeatureSDK.Model; @@ -22,8 +23,8 @@ public abstract class FeatureProvider /// error (if applicable): Provider, Invocation, Client, API /// finally: Provider, Invocation, Client, API /// - /// - public virtual IReadOnlyList GetProviderHooks() => Array.Empty(); + /// Immutable list of hooks + public virtual IImmutableList GetProviderHooks() => ImmutableList.Empty; /// /// Metadata describing the provider. diff --git a/src/OpenFeatureSDK/Model/FlagEvaluationOptions.cs b/src/OpenFeatureSDK/Model/FlagEvaluationOptions.cs index cb8bc353..b29b3e95 100644 --- a/src/OpenFeatureSDK/Model/FlagEvaluationOptions.cs +++ b/src/OpenFeatureSDK/Model/FlagEvaluationOptions.cs @@ -1,4 +1,5 @@ using System.Collections.Generic; +using System.Collections.Immutable; namespace OpenFeatureSDK.Model { @@ -12,22 +13,22 @@ public class FlagEvaluationOptions /// /// A immutable list of /// - public IReadOnlyList Hooks { get; } + public IImmutableList Hooks { get; } /// /// A immutable dictionary of hook hints /// - public IReadOnlyDictionary HookHints { get; } + public IImmutableDictionary HookHints { get; } /// /// Initializes a new instance of the class. /// /// /// Optional - a list of hints that are passed through the hook lifecycle - public FlagEvaluationOptions(IReadOnlyList hooks, IReadOnlyDictionary hookHints = null) + public FlagEvaluationOptions(IImmutableList hooks, IImmutableDictionary hookHints = null) { this.Hooks = hooks; - this.HookHints = hookHints ?? new Dictionary(); + this.HookHints = hookHints ?? ImmutableDictionary.Empty; } /// @@ -35,10 +36,10 @@ public FlagEvaluationOptions(IReadOnlyList hooks, IReadOnlyDictionary /// /// Optional - a list of hints that are passed through the hook lifecycle - public FlagEvaluationOptions(Hook hook, IReadOnlyDictionary hookHints = null) + public FlagEvaluationOptions(Hook hook, ImmutableDictionary hookHints = null) { - this.Hooks = new[] { hook }; - this.HookHints = hookHints ?? new Dictionary(); + this.Hooks = ImmutableList.Create(hook); + this.HookHints = hookHints ?? ImmutableDictionary.Empty; } } } diff --git a/src/OpenFeatureSDK/OpenFeature.cs b/src/OpenFeatureSDK/OpenFeature.cs index 42b23dcf..baa04c46 100644 --- a/src/OpenFeatureSDK/OpenFeature.cs +++ b/src/OpenFeatureSDK/OpenFeature.cs @@ -1,4 +1,6 @@ +using System.Collections.Concurrent; using System.Collections.Generic; +using System.Linq; using Microsoft.Extensions.Logging; using OpenFeatureSDK.Model; @@ -13,7 +15,7 @@ public sealed class OpenFeature { private EvaluationContext _evaluationContext = EvaluationContext.Empty; private FeatureProvider _featureProvider = new NoOpFeatureProvider(); - private readonly List _hooks = new List(); + private readonly ConcurrentStack _hooks = new ConcurrentStack(); /// /// Singleton instance of OpenFeature @@ -59,19 +61,19 @@ public FeatureClient GetClient(string name = null, string version = null, ILogge /// Appends list of hooks to global hooks list /// /// A list of - public void AddHooks(IEnumerable hooks) => this._hooks.AddRange(hooks); + public void AddHooks(IEnumerable hooks) => this._hooks.PushRange(hooks.ToArray()); /// /// Adds a hook to global hooks list /// /// A list of - public void AddHooks(Hook hook) => this._hooks.Add(hook); + public void AddHooks(Hook hook) => this._hooks.Push(hook); /// /// Returns the global immutable hooks list /// /// A immutable list of - public IReadOnlyList GetHooks() => this._hooks.AsReadOnly(); + public IEnumerable GetHooks() => this._hooks; /// /// Removes all hooks from global hooks list diff --git a/test/OpenFeatureSDK.Tests/OpenFeatureClientTests.cs b/test/OpenFeatureSDK.Tests/OpenFeatureClientTests.cs index 6babbdf5..570ba16d 100644 --- a/test/OpenFeatureSDK.Tests/OpenFeatureClientTests.cs +++ b/test/OpenFeatureSDK.Tests/OpenFeatureClientTests.cs @@ -1,5 +1,6 @@ using System; using System.Collections.Generic; +using System.Collections.Immutable; using System.Threading.Tasks; using AutoFixture; using FluentAssertions; @@ -68,7 +69,7 @@ public async Task OpenFeatureClient_Should_Allow_Flag_Evaluation() var defaultIntegerValue = fixture.Create(); var defaultDoubleValue = fixture.Create(); var defaultStructureValue = fixture.Create(); - var emptyFlagOptions = new FlagEvaluationOptions(new List(), new Dictionary()); + var emptyFlagOptions = new FlagEvaluationOptions(ImmutableList.Empty, ImmutableDictionary.Empty); OpenFeature.Instance.SetProvider(new NoOpFeatureProvider()); var client = OpenFeature.Instance.GetClient(clientName, clientVersion); @@ -114,7 +115,7 @@ public async Task OpenFeatureClient_Should_Allow_Details_Flag_Evaluation() var defaultIntegerValue = fixture.Create(); var defaultDoubleValue = fixture.Create(); var defaultStructureValue = fixture.Create(); - var emptyFlagOptions = new FlagEvaluationOptions(new List(), new Dictionary()); + var emptyFlagOptions = new FlagEvaluationOptions(ImmutableList.Empty, ImmutableDictionary.Empty); OpenFeature.Instance.SetProvider(new NoOpFeatureProvider()); var client = OpenFeature.Instance.GetClient(clientName, clientVersion); @@ -169,7 +170,7 @@ public async Task OpenFeatureClient_Should_Return_DefaultValue_When_Type_Mismatc mockedFeatureProvider.Setup(x => x.GetMetadata()) .Returns(new Metadata(fixture.Create())); mockedFeatureProvider.Setup(x => x.GetProviderHooks()) - .Returns(Array.Empty()); + .Returns(ImmutableList.Empty); OpenFeature.Instance.SetProvider(mockedFeatureProvider.Object); var client = OpenFeature.Instance.GetClient(clientName, clientVersion, mockedLogger.Object); @@ -206,7 +207,7 @@ public async Task Should_Resolve_BooleanValue() featureProviderMock.Setup(x => x.GetMetadata()) .Returns(new Metadata(fixture.Create())); featureProviderMock.Setup(x => x.GetProviderHooks()) - .Returns(Array.Empty()); + .Returns(ImmutableList.Empty); OpenFeature.Instance.SetProvider(featureProviderMock.Object); var client = OpenFeature.Instance.GetClient(clientName, clientVersion); @@ -232,7 +233,7 @@ public async Task Should_Resolve_StringValue() featureProviderMock.Setup(x => x.GetMetadata()) .Returns(new Metadata(fixture.Create())); featureProviderMock.Setup(x => x.GetProviderHooks()) - .Returns(Array.Empty()); + .Returns(ImmutableList.Empty); OpenFeature.Instance.SetProvider(featureProviderMock.Object); var client = OpenFeature.Instance.GetClient(clientName, clientVersion); @@ -258,7 +259,7 @@ public async Task Should_Resolve_IntegerValue() featureProviderMock.Setup(x => x.GetMetadata()) .Returns(new Metadata(fixture.Create())); featureProviderMock.Setup(x => x.GetProviderHooks()) - .Returns(Array.Empty()); + .Returns(ImmutableList.Empty); OpenFeature.Instance.SetProvider(featureProviderMock.Object); var client = OpenFeature.Instance.GetClient(clientName, clientVersion); @@ -284,7 +285,7 @@ public async Task Should_Resolve_DoubleValue() featureProviderMock.Setup(x => x.GetMetadata()) .Returns(new Metadata(fixture.Create())); featureProviderMock.Setup(x => x.GetProviderHooks()) - .Returns(Array.Empty()); + .Returns(ImmutableList.Empty); OpenFeature.Instance.SetProvider(featureProviderMock.Object); var client = OpenFeature.Instance.GetClient(clientName, clientVersion); @@ -310,7 +311,7 @@ public async Task Should_Resolve_StructureValue() featureProviderMock.Setup(x => x.GetMetadata()) .Returns(new Metadata(fixture.Create())); featureProviderMock.Setup(x => x.GetProviderHooks()) - .Returns(Array.Empty()); + .Returns(ImmutableList.Empty); OpenFeature.Instance.SetProvider(featureProviderMock.Object); var client = OpenFeature.Instance.GetClient(clientName, clientVersion); @@ -338,7 +339,7 @@ public async Task When_Error_Is_Returned_From_Provider_Should_Return_Error() featureProviderMock.Setup(x => x.GetMetadata()) .Returns(new Metadata(fixture.Create())); featureProviderMock.Setup(x => x.GetProviderHooks()) - .Returns(Array.Empty()); + .Returns(ImmutableList.Empty); OpenFeature.Instance.SetProvider(featureProviderMock.Object); var client = OpenFeature.Instance.GetClient(clientName, clientVersion); @@ -367,7 +368,7 @@ public async Task When_Exception_Occurs_During_Evaluation_Should_Return_Error() featureProviderMock.Setup(x => x.GetMetadata()) .Returns(new Metadata(fixture.Create())); featureProviderMock.Setup(x => x.GetProviderHooks()) - .Returns(Array.Empty()); + .Returns(ImmutableList.Empty); OpenFeature.Instance.SetProvider(featureProviderMock.Object); var client = OpenFeature.Instance.GetClient(clientName, clientVersion); diff --git a/test/OpenFeatureSDK.Tests/OpenFeatureHookTests.cs b/test/OpenFeatureSDK.Tests/OpenFeatureHookTests.cs index 0c97e18b..a8abccf7 100644 --- a/test/OpenFeatureSDK.Tests/OpenFeatureHookTests.cs +++ b/test/OpenFeatureSDK.Tests/OpenFeatureHookTests.cs @@ -1,5 +1,6 @@ using System; using System.Collections.Generic; +using System.Collections.Immutable; using System.Threading.Tasks; using AutoFixture; using FluentAssertions; @@ -83,7 +84,7 @@ public async Task Hooks_Should_Be_Called_In_Order() client.AddHooks(clientHook.Object); await client.GetBooleanValue(flagName, defaultValue, EvaluationContext.Empty, - new FlagEvaluationOptions(invocationHook.Object, new Dictionary())); + new FlagEvaluationOptions(invocationHook.Object, ImmutableDictionary.Empty)); apiHook.Verify(x => x.Before( It.IsAny>(), It.IsAny>()), Times.Once); @@ -173,19 +174,19 @@ public async Task Evaluation_Context_Must_Be_Mutable_Before_Hook() FlagValueType.Boolean, new ClientMetadata("test", "1.0.0"), new Metadata(NoOpProvider.NoOpProviderName), evaluationContext); - hook1.Setup(x => x.Before(It.IsAny>(), It.IsAny>())) + hook1.Setup(x => x.Before(It.IsAny>(), It.IsAny>())) .ReturnsAsync(evaluationContext); hook2.Setup(x => - x.Before(hookContext, It.IsAny>())) + x.Before(hookContext, It.IsAny>())) .ReturnsAsync(evaluationContext); var client = OpenFeature.Instance.GetClient("test", "1.0.0"); await client.GetBooleanValue("test", false, EvaluationContext.Empty, - new FlagEvaluationOptions(new[] { hook1.Object, hook2.Object }, new Dictionary())); + new FlagEvaluationOptions(ImmutableList.Create(hook1.Object, hook2.Object), ImmutableDictionary.Empty)); - hook1.Verify(x => x.Before(It.IsAny>(), It.IsAny>()), Times.Once); - hook2.Verify(x => x.Before(It.Is>(a => a.EvaluationContext.GetValue("test").AsString == "test"), It.IsAny>()), Times.Once); + hook1.Verify(x => x.Before(It.IsAny>(), It.IsAny>()), Times.Once); + hook2.Verify(x => x.Before(It.Is>(a => a.EvaluationContext.GetValue("test").AsString == "test"), It.IsAny>()), Times.Once); } [Fact] @@ -232,7 +233,7 @@ public async Task Evaluation_Context_Must_Be_Merged_In_Correct_Order() .Returns(new Metadata(null)); provider.Setup(x => x.GetProviderHooks()) - .Returns(Array.Empty()); + .Returns(ImmutableList.Empty); provider.Setup(x => x.ResolveBooleanValue(It.IsAny(), It.IsAny(), It.IsAny())) .ReturnsAsync(new ResolutionDetails("test", true)); @@ -240,12 +241,12 @@ public async Task Evaluation_Context_Must_Be_Merged_In_Correct_Order() OpenFeature.Instance.SetProvider(provider.Object); var hook = new Mock(MockBehavior.Strict); - hook.Setup(x => x.Before(It.IsAny>(), It.IsAny>())) + hook.Setup(x => x.Before(It.IsAny>(), It.IsAny>())) .ReturnsAsync(hookContext); var client = OpenFeature.Instance.GetClient("test", "1.0.0", null, clientContext); - await client.GetBooleanValue("test", false, invocationContext, new FlagEvaluationOptions(new[] { hook.Object }, new Dictionary())); + await client.GetBooleanValue("test", false, invocationContext, new FlagEvaluationOptions(ImmutableList.Create(hook.Object), ImmutableDictionary.Empty)); // after proper merging, all properties should equal true provider.Verify(x => x.ResolveBooleanValue(It.IsAny(), It.IsAny(), It.Is(y => @@ -307,7 +308,7 @@ public async Task Hook_Should_Execute_In_Correct_Order() .Returns(new Metadata(null)); featureProvider.Setup(x => x.GetProviderHooks()) - .Returns(Array.Empty()); + .Returns(ImmutableList.Empty); hook.InSequence(sequence).Setup(x => x.Before(It.IsAny>(), It.IsAny>())) @@ -351,9 +352,9 @@ public async Task Register_Hooks_Should_Be_Available_At_All_Levels() var client = OpenFeature.Instance.GetClient(); client.AddHooks(hook2.Object); await client.GetBooleanValue("test", false, null, - new FlagEvaluationOptions(hook3.Object, new Dictionary())); + new FlagEvaluationOptions(hook3.Object, ImmutableDictionary.Empty)); - OpenFeature.Instance.GetHooks().Count.Should().Be(1); + Assert.Single(OpenFeature.Instance.GetHooks()); client.GetHooks().Count.Should().Be(1); testProvider.GetProviderHooks().Count.Should().Be(1); } @@ -372,7 +373,7 @@ public async Task Finally_Hook_Should_Be_Executed_Even_If_Abnormal_Termination() .Returns(new Metadata(null)); featureProvider.Setup(x => x.GetProviderHooks()) - .Returns(Array.Empty()); + .Returns(ImmutableList.Empty); hook1.InSequence(sequence).Setup(x => x.Before(It.IsAny>(), null)) @@ -431,7 +432,7 @@ public async Task Error_Hook_Should_Be_Executed_Even_If_Abnormal_Termination() featureProvider1.Setup(x => x.GetMetadata()) .Returns(new Metadata(null)); featureProvider1.Setup(x => x.GetProviderHooks()) - .Returns(Array.Empty()); + .Returns(ImmutableList.Empty); hook1.InSequence(sequence).Setup(x => x.Before(It.IsAny>(), null)) @@ -478,7 +479,7 @@ public async Task Error_Occurs_During_Before_After_Evaluation_Should_Not_Invoke_ featureProvider.Setup(x => x.GetMetadata()) .Returns(new Metadata(null)); featureProvider.Setup(x => x.GetProviderHooks()) - .Returns(Array.Empty()); + .Returns(ImmutableList.Empty); hook1.InSequence(sequence).Setup(x => x.Before(It.IsAny>(), It.IsAny>())) @@ -518,7 +519,7 @@ public async Task Hook_Hints_May_Be_Optional() .Returns(new Metadata(null)); featureProvider.Setup(x => x.GetProviderHooks()) - .Returns(Array.Empty()); + .Returns(ImmutableList.Empty); hook.InSequence(sequence) .Setup(x => x.Before(It.IsAny>(), defaultEmptyHookHints)) diff --git a/test/OpenFeatureSDK.Tests/OpenFeatureTests.cs b/test/OpenFeatureSDK.Tests/OpenFeatureTests.cs index 5f9a778b..bf8edc5c 100644 --- a/test/OpenFeatureSDK.Tests/OpenFeatureTests.cs +++ b/test/OpenFeatureSDK.Tests/OpenFeatureTests.cs @@ -1,3 +1,4 @@ +using System.Linq; using FluentAssertions; using Moq; using OpenFeatureSDK.Constant; @@ -34,18 +35,18 @@ public void OpenFeature_Should_Add_Hooks() openFeature.AddHooks(hook1); openFeature.GetHooks().Should().Contain(hook1); - openFeature.GetHooks().Count.Should().Be(1); + Assert.Single(openFeature.GetHooks()); openFeature.AddHooks(hook2); - openFeature.GetHooks().Should().ContainInOrder(hook1, hook2); - openFeature.GetHooks().Count.Should().Be(2); + openFeature.GetHooks().Should().ContainInOrder(hook2, hook1); + openFeature.GetHooks().Count().Should().Be(2); openFeature.AddHooks(new[] { hook3, hook4 }); - openFeature.GetHooks().Should().ContainInOrder(hook1, hook2, hook3, hook4); - openFeature.GetHooks().Count.Should().Be(4); + openFeature.GetHooks().Should().ContainInOrder(hook4, hook3, hook2, hook1); + openFeature.GetHooks().Count().Should().Be(4); openFeature.ClearHooks(); - openFeature.GetHooks().Count.Should().Be(0); + Assert.Empty(openFeature.GetHooks()); } [Fact] diff --git a/test/OpenFeatureSDK.Tests/TestImplementations.cs b/test/OpenFeatureSDK.Tests/TestImplementations.cs index 4236077c..2da88c7b 100644 --- a/test/OpenFeatureSDK.Tests/TestImplementations.cs +++ b/test/OpenFeatureSDK.Tests/TestImplementations.cs @@ -1,5 +1,6 @@ using System; using System.Collections.Generic; +using System.Collections.Immutable; using System.Threading.Tasks; using OpenFeatureSDK.Model; @@ -39,7 +40,7 @@ public class TestProvider : FeatureProvider public void AddHook(Hook hook) => this._hooks.Add(hook); - public override IReadOnlyList GetProviderHooks() => this._hooks.AsReadOnly(); + public override IImmutableList GetProviderHooks() => this._hooks.ToImmutableList(); public override Metadata GetMetadata() { From 08e68ba1cd2a7b4285a03d551dc9d5a848a2de06 Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Fri, 7 Oct 2022 15:10:29 -0700 Subject: [PATCH 02/13] Make references volatile, always use the global provider instead of the one at the time of client instantiation. Signed-off-by: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> --- src/OpenFeatureSDK/FeatureProvider.cs | 2 -- .../Model/FlagEvaluationOptions.cs | 1 - src/OpenFeatureSDK/OpenFeature.cs | 12 +++++++++--- src/OpenFeatureSDK/OpenFeatureClient.cs | 17 +++++++---------- .../OpenFeatureClientTests.cs | 7 ------- 5 files changed, 16 insertions(+), 23 deletions(-) diff --git a/src/OpenFeatureSDK/FeatureProvider.cs b/src/OpenFeatureSDK/FeatureProvider.cs index ed9a79b5..e2934cea 100644 --- a/src/OpenFeatureSDK/FeatureProvider.cs +++ b/src/OpenFeatureSDK/FeatureProvider.cs @@ -1,5 +1,3 @@ -using System; -using System.Collections.Generic; using System.Collections.Immutable; using System.Threading.Tasks; using OpenFeatureSDK.Model; diff --git a/src/OpenFeatureSDK/Model/FlagEvaluationOptions.cs b/src/OpenFeatureSDK/Model/FlagEvaluationOptions.cs index b29b3e95..680c5154 100644 --- a/src/OpenFeatureSDK/Model/FlagEvaluationOptions.cs +++ b/src/OpenFeatureSDK/Model/FlagEvaluationOptions.cs @@ -1,4 +1,3 @@ -using System.Collections.Generic; using System.Collections.Immutable; namespace OpenFeatureSDK.Model diff --git a/src/OpenFeatureSDK/OpenFeature.cs b/src/OpenFeatureSDK/OpenFeature.cs index baa04c46..321dd2ab 100644 --- a/src/OpenFeatureSDK/OpenFeature.cs +++ b/src/OpenFeatureSDK/OpenFeature.cs @@ -13,8 +13,14 @@ namespace OpenFeatureSDK /// public sealed class OpenFeature { - private EvaluationContext _evaluationContext = EvaluationContext.Empty; - private FeatureProvider _featureProvider = new NoOpFeatureProvider(); + /// References are atomic in C#, but it is possible to get an old value due to register caching. + /// Making it volatile will result in reads bypassing that cache. It doesn't provide any ordering + /// guarantee. Considering this is just a method to replace the value, regardless of the current value, + /// this should be sufficient. + /// https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/language-specification/variables#96-atomicity-of-variable-references + /// https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/keywords/volatile + private volatile EvaluationContext _evaluationContext = EvaluationContext.Empty; + private volatile FeatureProvider _featureProvider = new NoOpFeatureProvider(); private readonly ConcurrentStack _hooks = new ConcurrentStack(); /// @@ -55,7 +61,7 @@ private OpenFeature() { } /// Context given to this client /// public FeatureClient GetClient(string name = null, string version = null, ILogger logger = null, EvaluationContext context = null) => - new FeatureClient(this._featureProvider, name, version, logger, context); + new FeatureClient(name, version, logger, context); /// /// Appends list of hooks to global hooks list diff --git a/src/OpenFeatureSDK/OpenFeatureClient.cs b/src/OpenFeatureSDK/OpenFeatureClient.cs index 3d4120b7..9d833b26 100644 --- a/src/OpenFeatureSDK/OpenFeatureClient.cs +++ b/src/OpenFeatureSDK/OpenFeatureClient.cs @@ -17,7 +17,6 @@ namespace OpenFeatureSDK public sealed class FeatureClient : IFeatureClient { private readonly ClientMetadata _metadata; - private readonly FeatureProvider _featureProvider; private readonly List _hooks = new List(); private readonly ILogger _logger; private EvaluationContext _evaluationContext; @@ -36,15 +35,13 @@ public sealed class FeatureClient : IFeatureClient /// /// Initializes a new instance of the class. /// - /// Feature provider used by client /// Name of client /// Version of client /// Logger used by client /// Context given to this client /// Throws if any of the required parameters are null - public FeatureClient(FeatureProvider featureProvider, string name, string version, ILogger logger = null, EvaluationContext context = null) + 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(new NullLoggerFactory()); this._evaluationContext = context ?? EvaluationContext.Empty; @@ -101,7 +98,7 @@ public async Task GetBooleanValue(string flagKey, bool defaultValue, Evalu /// Resolved flag details public async Task> GetBooleanDetails(string flagKey, bool defaultValue, EvaluationContext context = null, FlagEvaluationOptions config = null) => - await this.EvaluateFlag(this._featureProvider.ResolveBooleanValue, FlagValueType.Boolean, flagKey, + await this.EvaluateFlag(OpenFeature.Instance.GetProvider().ResolveBooleanValue, FlagValueType.Boolean, flagKey, defaultValue, context, config); /// @@ -126,7 +123,7 @@ public async Task GetStringValue(string flagKey, string defaultValue, Ev /// Resolved flag details public async Task> GetStringDetails(string flagKey, string defaultValue, EvaluationContext context = null, FlagEvaluationOptions config = null) => - await this.EvaluateFlag(this._featureProvider.ResolveStringValue, FlagValueType.String, flagKey, + await this.EvaluateFlag(OpenFeature.Instance.GetProvider().ResolveStringValue, FlagValueType.String, flagKey, defaultValue, context, config); /// @@ -151,7 +148,7 @@ public async Task GetIntegerValue(string flagKey, int defaultValue, Evaluat /// Resolved flag details public async Task> GetIntegerDetails(string flagKey, int defaultValue, EvaluationContext context = null, FlagEvaluationOptions config = null) => - await this.EvaluateFlag(this._featureProvider.ResolveIntegerValue, FlagValueType.Number, flagKey, + await this.EvaluateFlag(OpenFeature.Instance.GetProvider().ResolveIntegerValue, FlagValueType.Number, flagKey, defaultValue, context, config); /// @@ -177,7 +174,7 @@ public async Task GetDoubleValue(string flagKey, double defaultValue, /// Resolved flag details public async Task> GetDoubleDetails(string flagKey, double defaultValue, EvaluationContext context = null, FlagEvaluationOptions config = null) => - await this.EvaluateFlag(this._featureProvider.ResolveDoubleValue, FlagValueType.Number, flagKey, + await this.EvaluateFlag(OpenFeature.Instance.GetProvider().ResolveDoubleValue, FlagValueType.Number, flagKey, defaultValue, context, config); /// @@ -202,7 +199,7 @@ public async Task GetObjectValue(string flagKey, Value defaultValue, Eval /// Resolved flag details public async Task> GetObjectDetails(string flagKey, Value defaultValue, EvaluationContext context = null, FlagEvaluationOptions config = null) => - await this.EvaluateFlag(this._featureProvider.ResolveStructureValue, FlagValueType.Object, flagKey, + await this.EvaluateFlag(OpenFeature.Instance.GetProvider().ResolveStructureValue, FlagValueType.Object, flagKey, defaultValue, context, config); private async Task> EvaluateFlag( @@ -227,7 +224,7 @@ private async Task> EvaluateFlag( .Concat(OpenFeature.Instance.GetHooks()) .Concat(this._hooks) .Concat(options?.Hooks ?? Enumerable.Empty()) - .Concat(this._featureProvider.GetProviderHooks()) + .Concat(OpenFeature.Instance.GetProvider().GetProviderHooks()) .ToList() .AsReadOnly(); diff --git a/test/OpenFeatureSDK.Tests/OpenFeatureClientTests.cs b/test/OpenFeatureSDK.Tests/OpenFeatureClientTests.cs index 570ba16d..6815cd92 100644 --- a/test/OpenFeatureSDK.Tests/OpenFeatureClientTests.cs +++ b/test/OpenFeatureSDK.Tests/OpenFeatureClientTests.cs @@ -380,13 +380,6 @@ public async Task When_Exception_Occurs_During_Evaluation_Should_Return_Error() featureProviderMock.Verify(x => x.ResolveStructureValue(flagName, defaultValue, It.IsAny()), Times.Once); } - [Fact] - public void Should_Throw_ArgumentNullException_When_Provider_Is_Null() - { - TestProvider provider = null; - Assert.Throws(() => new FeatureClient(provider, "test", "test")); - } - [Fact] public void Should_Get_And_Set_Context() { From bd7ba04e216727ed393069215f0d262a4a4ab4e9 Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Fri, 7 Oct 2022 15:33:27 -0700 Subject: [PATCH 03/13] Reverse hooks. Make hooks in FeatureClient a ConcurrentStack. Signed-off-by: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> --- src/OpenFeatureSDK/Model/FlagEvaluationOptions.cs | 4 ++-- src/OpenFeatureSDK/OpenFeature.cs | 2 +- src/OpenFeatureSDK/OpenFeatureClient.cs | 11 ++++++----- test/OpenFeatureSDK.Tests/OpenFeatureClientTests.cs | 7 ++++--- test/OpenFeatureSDK.Tests/OpenFeatureHookTests.cs | 5 +++-- test/OpenFeatureSDK.Tests/OpenFeatureTests.cs | 4 ++-- 6 files changed, 18 insertions(+), 15 deletions(-) diff --git a/src/OpenFeatureSDK/Model/FlagEvaluationOptions.cs b/src/OpenFeatureSDK/Model/FlagEvaluationOptions.cs index 680c5154..38396723 100644 --- a/src/OpenFeatureSDK/Model/FlagEvaluationOptions.cs +++ b/src/OpenFeatureSDK/Model/FlagEvaluationOptions.cs @@ -22,7 +22,7 @@ public class FlagEvaluationOptions /// /// Initializes a new instance of the class. /// - /// + /// An immutable list of hooks to use during evaluation /// Optional - a list of hints that are passed through the hook lifecycle public FlagEvaluationOptions(IImmutableList hooks, IImmutableDictionary hookHints = null) { @@ -33,7 +33,7 @@ public FlagEvaluationOptions(IImmutableList hooks, IImmutableDictionary /// Initializes a new instance of the class. /// - /// + /// A hook to use during the evaluation /// Optional - a list of hints that are passed through the hook lifecycle public FlagEvaluationOptions(Hook hook, ImmutableDictionary hookHints = null) { diff --git a/src/OpenFeatureSDK/OpenFeature.cs b/src/OpenFeatureSDK/OpenFeature.cs index 321dd2ab..a6bdc2b4 100644 --- a/src/OpenFeatureSDK/OpenFeature.cs +++ b/src/OpenFeatureSDK/OpenFeature.cs @@ -79,7 +79,7 @@ public FeatureClient GetClient(string name = null, string version = null, ILogge /// Returns the global immutable hooks list /// /// A immutable list of - public IEnumerable GetHooks() => this._hooks; + public IEnumerable GetHooks() => this._hooks.Reverse(); /// /// Removes all hooks from global hooks list diff --git a/src/OpenFeatureSDK/OpenFeatureClient.cs b/src/OpenFeatureSDK/OpenFeatureClient.cs index 9d833b26..1f49379c 100644 --- a/src/OpenFeatureSDK/OpenFeatureClient.cs +++ b/src/OpenFeatureSDK/OpenFeatureClient.cs @@ -1,4 +1,5 @@ using System; +using System.Collections.Concurrent; using System.Collections.Generic; using System.Linq; using System.Threading.Tasks; @@ -17,7 +18,7 @@ namespace OpenFeatureSDK public sealed class FeatureClient : IFeatureClient { private readonly ClientMetadata _metadata; - private readonly List _hooks = new List(); + private readonly ConcurrentStack _hooks = new ConcurrentStack(); private readonly ILogger _logger; private EvaluationContext _evaluationContext; @@ -57,19 +58,19 @@ public FeatureClient(string name, string version, ILogger logger = null, Evaluat /// Add hook to client /// /// Hook that implements the interface - public void AddHooks(Hook hook) => this._hooks.Add(hook); + public void AddHooks(Hook hook) => this._hooks.Push(hook); /// /// Appends hooks to client /// /// A list of Hooks that implement the interface - public void AddHooks(IEnumerable hooks) => this._hooks.AddRange(hooks); + public void AddHooks(IEnumerable hooks) => this._hooks.PushRange(hooks.ToArray()); /// /// Return a immutable list of hooks that are registered against the client /// /// A list of immutable hooks - public IReadOnlyList GetHooks() => this._hooks.ToList().AsReadOnly(); + public IEnumerable GetHooks() => this._hooks.Reverse(); /// /// Removes all hooks from the client @@ -222,7 +223,7 @@ private async Task> EvaluateFlag( var allHooks = new List() .Concat(OpenFeature.Instance.GetHooks()) - .Concat(this._hooks) + .Concat(this.GetHooks()) .Concat(options?.Hooks ?? Enumerable.Empty()) .Concat(OpenFeature.Instance.GetProvider().GetProviderHooks()) .ToList() diff --git a/test/OpenFeatureSDK.Tests/OpenFeatureClientTests.cs b/test/OpenFeatureSDK.Tests/OpenFeatureClientTests.cs index 6815cd92..1a13b85b 100644 --- a/test/OpenFeatureSDK.Tests/OpenFeatureClientTests.cs +++ b/test/OpenFeatureSDK.Tests/OpenFeatureClientTests.cs @@ -1,6 +1,7 @@ using System; using System.Collections.Generic; using System.Collections.Immutable; +using System.Linq; using System.Threading.Tasks; using AutoFixture; using FluentAssertions; @@ -31,14 +32,14 @@ public void OpenFeatureClient_Should_Allow_Hooks() client.AddHooks(new[] { hook1, hook2 }); client.GetHooks().Should().ContainInOrder(hook1, hook2); - client.GetHooks().Count.Should().Be(2); + client.GetHooks().Count().Should().Be(2); client.AddHooks(hook3); client.GetHooks().Should().ContainInOrder(hook1, hook2, hook3); - client.GetHooks().Count.Should().Be(3); + client.GetHooks().Count().Should().Be(3); client.ClearHooks(); - client.GetHooks().Count.Should().Be(0); + Assert.Empty(client.GetHooks()); } [Fact] diff --git a/test/OpenFeatureSDK.Tests/OpenFeatureHookTests.cs b/test/OpenFeatureSDK.Tests/OpenFeatureHookTests.cs index a8abccf7..3b13e6b8 100644 --- a/test/OpenFeatureSDK.Tests/OpenFeatureHookTests.cs +++ b/test/OpenFeatureSDK.Tests/OpenFeatureHookTests.cs @@ -1,6 +1,7 @@ using System; using System.Collections.Generic; using System.Collections.Immutable; +using System.Linq; using System.Threading.Tasks; using AutoFixture; using FluentAssertions; @@ -355,7 +356,7 @@ await client.GetBooleanValue("test", false, null, new FlagEvaluationOptions(hook3.Object, ImmutableDictionary.Empty)); Assert.Single(OpenFeature.Instance.GetHooks()); - client.GetHooks().Count.Should().Be(1); + client.GetHooks().Count().Should().Be(1); testProvider.GetProviderHooks().Count.Should().Be(1); } @@ -406,7 +407,7 @@ public async Task Finally_Hook_Should_Be_Executed_Even_If_Abnormal_Termination() OpenFeature.Instance.SetProvider(featureProvider.Object); var client = OpenFeature.Instance.GetClient(); client.AddHooks(new[] { hook1.Object, hook2.Object }); - client.GetHooks().Count.Should().Be(2); + client.GetHooks().Count().Should().Be(2); await client.GetBooleanValue("test", false); diff --git a/test/OpenFeatureSDK.Tests/OpenFeatureTests.cs b/test/OpenFeatureSDK.Tests/OpenFeatureTests.cs index bf8edc5c..8a96e84b 100644 --- a/test/OpenFeatureSDK.Tests/OpenFeatureTests.cs +++ b/test/OpenFeatureSDK.Tests/OpenFeatureTests.cs @@ -38,11 +38,11 @@ public void OpenFeature_Should_Add_Hooks() Assert.Single(openFeature.GetHooks()); openFeature.AddHooks(hook2); - openFeature.GetHooks().Should().ContainInOrder(hook2, hook1); + openFeature.GetHooks().Should().ContainInOrder(hook1, hook2); openFeature.GetHooks().Count().Should().Be(2); openFeature.AddHooks(new[] { hook3, hook4 }); - openFeature.GetHooks().Should().ContainInOrder(hook4, hook3, hook2, hook1); + openFeature.GetHooks().Should().ContainInOrder(hook1, hook2, hook3, hook4); openFeature.GetHooks().Count().Should().Be(4); openFeature.ClearHooks(); From dda0f28da9be04d29de5c1d53d621d1ad1d80329 Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Fri, 7 Oct 2022 15:37:07 -0700 Subject: [PATCH 04/13] Update documentation on GetHooks methods. Signed-off-by: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> --- src/OpenFeatureSDK/OpenFeature.cs | 9 +++++++-- src/OpenFeatureSDK/OpenFeatureClient.cs | 9 +++++++-- 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/src/OpenFeatureSDK/OpenFeature.cs b/src/OpenFeatureSDK/OpenFeature.cs index a6bdc2b4..909c49c3 100644 --- a/src/OpenFeatureSDK/OpenFeature.cs +++ b/src/OpenFeatureSDK/OpenFeature.cs @@ -76,9 +76,14 @@ public FeatureClient GetClient(string name = null, string version = null, ILogge public void AddHooks(Hook hook) => this._hooks.Push(hook); /// - /// Returns the global immutable hooks list + /// Enumerates the global hooks. + /// + /// The items enumerated will reflect the registered hooks + /// at the start of enumeration. Hooks added during enumeration + /// will not be included. + /// /// - /// A immutable list of + /// Enumeration of public IEnumerable GetHooks() => this._hooks.Reverse(); /// diff --git a/src/OpenFeatureSDK/OpenFeatureClient.cs b/src/OpenFeatureSDK/OpenFeatureClient.cs index 1f49379c..db9aa9a6 100644 --- a/src/OpenFeatureSDK/OpenFeatureClient.cs +++ b/src/OpenFeatureSDK/OpenFeatureClient.cs @@ -67,9 +67,14 @@ public FeatureClient(string name, string version, ILogger logger = null, Evaluat public void AddHooks(IEnumerable hooks) => this._hooks.PushRange(hooks.ToArray()); /// - /// Return a immutable list of hooks that are registered against the client + /// Enumerates the global hooks. + /// + /// The items enumerated will reflect the registered hooks + /// at the start of enumeration. Hooks added during enumeration + /// will not be included. + /// /// - /// A list of immutable hooks + /// Enumeration of public IEnumerable GetHooks() => this._hooks.Reverse(); /// From a86fb8371c8ab8395b72c311f0051c45ef4e9e07 Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Mon, 10 Oct 2022 09:39:59 -0700 Subject: [PATCH 05/13] Use the same provider reference for the duration of an evaluation. Signed-off-by: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> --- src/OpenFeatureSDK/OpenFeatureClient.cs | 45 ++++++++++++++++++++----- 1 file changed, 37 insertions(+), 8 deletions(-) diff --git a/src/OpenFeatureSDK/OpenFeatureClient.cs b/src/OpenFeatureSDK/OpenFeatureClient.cs index db9aa9a6..fef01fe4 100644 --- a/src/OpenFeatureSDK/OpenFeatureClient.cs +++ b/src/OpenFeatureSDK/OpenFeatureClient.cs @@ -22,6 +22,28 @@ public sealed class FeatureClient : IFeatureClient private readonly ILogger _logger; private EvaluationContext _evaluationContext; + /// + /// Get a provider and an associated typed flag resolution method. + /// + /// 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. + /// + /// + /// + /// This method should return the desired flag resolution method from the given provider reference. + /// + /// The type of the resolution method + /// A tuple containing a resolution method and the provider it came from. + private (Func>>, FeatureProvider) + ExtractProvider( + Func>>> 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); + } + /// /// Gets the EvaluationContext of this client /// @@ -104,7 +126,8 @@ public async Task GetBooleanValue(string flagKey, bool defaultValue, Evalu /// Resolved flag details public async Task> GetBooleanDetails(string flagKey, bool defaultValue, EvaluationContext context = null, FlagEvaluationOptions config = null) => - await this.EvaluateFlag(OpenFeature.Instance.GetProvider().ResolveBooleanValue, FlagValueType.Boolean, flagKey, + await this.EvaluateFlag(this.ExtractProvider(provider => provider.ResolveBooleanValue), + FlagValueType.Boolean, flagKey, defaultValue, context, config); /// @@ -129,7 +152,8 @@ public async Task GetStringValue(string flagKey, string defaultValue, Ev /// Resolved flag details public async Task> GetStringDetails(string flagKey, string defaultValue, EvaluationContext context = null, FlagEvaluationOptions config = null) => - await this.EvaluateFlag(OpenFeature.Instance.GetProvider().ResolveStringValue, FlagValueType.String, flagKey, + await this.EvaluateFlag(this.ExtractProvider(provider => provider.ResolveStringValue), + FlagValueType.String, flagKey, defaultValue, context, config); /// @@ -154,7 +178,8 @@ public async Task GetIntegerValue(string flagKey, int defaultValue, Evaluat /// Resolved flag details public async Task> GetIntegerDetails(string flagKey, int defaultValue, EvaluationContext context = null, FlagEvaluationOptions config = null) => - await this.EvaluateFlag(OpenFeature.Instance.GetProvider().ResolveIntegerValue, FlagValueType.Number, flagKey, + await this.EvaluateFlag(this.ExtractProvider(provider => provider.ResolveIntegerValue), + FlagValueType.Number, flagKey, defaultValue, context, config); /// @@ -180,7 +205,8 @@ public async Task GetDoubleValue(string flagKey, double defaultValue, /// Resolved flag details public async Task> GetDoubleDetails(string flagKey, double defaultValue, EvaluationContext context = null, FlagEvaluationOptions config = null) => - await this.EvaluateFlag(OpenFeature.Instance.GetProvider().ResolveDoubleValue, FlagValueType.Number, flagKey, + await this.EvaluateFlag(this.ExtractProvider(provider => provider.ResolveDoubleValue), + FlagValueType.Number, flagKey, defaultValue, context, config); /// @@ -205,14 +231,17 @@ public async Task GetObjectValue(string flagKey, Value defaultValue, Eval /// Resolved flag details public async Task> GetObjectDetails(string flagKey, Value defaultValue, EvaluationContext context = null, FlagEvaluationOptions config = null) => - await this.EvaluateFlag(OpenFeature.Instance.GetProvider().ResolveStructureValue, FlagValueType.Object, flagKey, + await this.EvaluateFlag(this.ExtractProvider(provider => provider.ResolveStructureValue), + FlagValueType.Object, flagKey, defaultValue, context, config); private async Task> EvaluateFlag( - Func>> resolveValueDelegate, + (Func>>, 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) { @@ -230,7 +259,7 @@ private async Task> EvaluateFlag( .Concat(OpenFeature.Instance.GetHooks()) .Concat(this.GetHooks()) .Concat(options?.Hooks ?? Enumerable.Empty()) - .Concat(OpenFeature.Instance.GetProvider().GetProviderHooks()) + .Concat(provider.GetProviderHooks()) .ToList() .AsReadOnly(); @@ -244,7 +273,7 @@ private async Task> EvaluateFlag( flagKey, defaultValue, flagValueType, this._metadata, - OpenFeature.Instance.GetProviderMetadata(), + provider.GetMetadata(), evaluationContextBuilder.Build() ); From 4a6736f147f4a63f4df8ed243a51c40c221308a6 Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Mon, 10 Oct 2022 10:12:54 -0700 Subject: [PATCH 06/13] Use slim read/write lock. Signed-off-by: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> --- src/OpenFeatureSDK/OpenFeature.cs | 69 ++++++++++++++++++++++++------- 1 file changed, 53 insertions(+), 16 deletions(-) diff --git a/src/OpenFeatureSDK/OpenFeature.cs b/src/OpenFeatureSDK/OpenFeature.cs index 909c49c3..8f2dd3eb 100644 --- a/src/OpenFeatureSDK/OpenFeature.cs +++ b/src/OpenFeatureSDK/OpenFeature.cs @@ -1,6 +1,7 @@ using System.Collections.Concurrent; using System.Collections.Generic; using System.Linq; +using System.Threading; using Microsoft.Extensions.Logging; using OpenFeatureSDK.Model; @@ -13,16 +14,13 @@ namespace OpenFeatureSDK /// public sealed class OpenFeature { - /// References are atomic in C#, but it is possible to get an old value due to register caching. - /// Making it volatile will result in reads bypassing that cache. It doesn't provide any ordering - /// guarantee. Considering this is just a method to replace the value, regardless of the current value, - /// this should be sufficient. - /// https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/language-specification/variables#96-atomicity-of-variable-references - /// https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/keywords/volatile - private volatile EvaluationContext _evaluationContext = EvaluationContext.Empty; - private volatile FeatureProvider _featureProvider = new NoOpFeatureProvider(); + private EvaluationContext _evaluationContext = EvaluationContext.Empty; + private FeatureProvider _featureProvider = new NoOpFeatureProvider(); private readonly ConcurrentStack _hooks = new ConcurrentStack(); + private readonly ReaderWriterLockSlim _evaluationContextLock = new ReaderWriterLockSlim(); + private readonly ReaderWriterLockSlim _featureProviderLock = new ReaderWriterLockSlim(); + /// /// Singleton instance of OpenFeature /// @@ -38,19 +36,35 @@ private OpenFeature() { } /// Sets the feature provider /// /// Implementation of - public void SetProvider(FeatureProvider featureProvider) => this._featureProvider = featureProvider; + public void SetProvider(FeatureProvider featureProvider) + { + this._featureProviderLock.EnterWriteLock(); + this._featureProvider = featureProvider; + this._featureProviderLock.ExitWriteLock(); + } /// /// Gets the feature provider /// /// - public FeatureProvider GetProvider() => this._featureProvider; + public FeatureProvider GetProvider() + { + this._featureProviderLock.EnterReadLock(); + try + { + return this._featureProvider; + } + finally + { + this._featureProviderLock.ExitReadLock(); + } + } /// /// Gets providers metadata /// /// - public Metadata GetProviderMetadata() => this._featureProvider.GetMetadata(); + public Metadata GetProviderMetadata() => this.GetProvider().GetMetadata(); /// /// Create a new instance of using the current provider @@ -60,7 +74,8 @@ private OpenFeature() { } /// Logger instance used by client /// Context given to this client /// - public FeatureClient GetClient(string name = null, string version = null, ILogger logger = null, EvaluationContext context = null) => + public FeatureClient GetClient(string name = null, string version = null, ILogger logger = null, + EvaluationContext context = null) => new FeatureClient(name, version, logger, context); /// @@ -94,13 +109,35 @@ public FeatureClient GetClient(string name = null, string version = null, ILogge /// /// Sets the global /// - /// - public void SetContext(EvaluationContext context) => this._evaluationContext = context ?? EvaluationContext.Empty; + /// The to set + public void SetContext(EvaluationContext context) + { + this._evaluationContextLock.EnterWriteLock(); + try + { + this._evaluationContext = context ?? EvaluationContext.Empty; + } + finally + { + this._evaluationContextLock.ExitWriteLock(); + } + } /// /// Gets the global /// - /// - public EvaluationContext GetContext() => this._evaluationContext; + /// An + public EvaluationContext GetContext() + { + this._evaluationContextLock.EnterReadLock(); + try + { + return this._evaluationContext; + } + finally + { + this._evaluationContextLock.ExitReadLock(); + } + } } } From 392a005737bb0d90e4a85e0ee09e3dc9ab2638fb Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Mon, 10 Oct 2022 10:13:44 -0700 Subject: [PATCH 07/13] Update to use try/finally. Signed-off-by: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> --- src/OpenFeatureSDK/OpenFeature.cs | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/OpenFeatureSDK/OpenFeature.cs b/src/OpenFeatureSDK/OpenFeature.cs index 8f2dd3eb..be53ebd6 100644 --- a/src/OpenFeatureSDK/OpenFeature.cs +++ b/src/OpenFeatureSDK/OpenFeature.cs @@ -39,8 +39,14 @@ private OpenFeature() { } public void SetProvider(FeatureProvider featureProvider) { this._featureProviderLock.EnterWriteLock(); - this._featureProvider = featureProvider; - this._featureProviderLock.ExitWriteLock(); + try + { + this._featureProvider = featureProvider; + } + finally + { + this._featureProviderLock.ExitWriteLock(); + } } /// From b75e3e33767133b33656ec5528fce7395750d0b8 Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Mon, 10 Oct 2022 10:23:07 -0700 Subject: [PATCH 08/13] Add documentation. Signed-off-by: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> --- src/OpenFeatureSDK/OpenFeature.cs | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/src/OpenFeatureSDK/OpenFeature.cs b/src/OpenFeatureSDK/OpenFeature.cs index be53ebd6..60c054a1 100644 --- a/src/OpenFeatureSDK/OpenFeature.cs +++ b/src/OpenFeatureSDK/OpenFeature.cs @@ -51,6 +51,13 @@ public void SetProvider(FeatureProvider featureProvider) /// /// Gets the feature provider + /// + /// The feature provider may be set from multiple threads, when accessing the global feature provider + /// it should be accessed once for an operation, and then that reference should be used for all dependent + /// operations. For instance, during an evaluation the flag resolution method, and the provider hooks + /// should be accessed from the same reference, not two independent calls to + /// . + /// /// /// public FeatureProvider GetProvider() @@ -68,6 +75,11 @@ public FeatureProvider GetProvider() /// /// Gets providers metadata + /// + /// This method is not guaranteed to return the same provider instance that may be used during an evaluation + /// in the case where the provider may be changed from another thread. + /// For multiple dependent provider operations see . + /// /// /// public Metadata GetProviderMetadata() => this.GetProvider().GetMetadata(); @@ -131,6 +143,12 @@ public void SetContext(EvaluationContext context) /// /// Gets the global + /// + /// The evaluation context may be set from multiple threads, when accessing the global evaluation context + /// it should be accessed once for an operation, and then that reference should be used for all dependent + /// operations. + /// . + /// /// /// An public EvaluationContext GetContext() From c7f2cc3924cc2668ed4c09f40c880a4b59215050 Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Mon, 10 Oct 2022 11:00:37 -0700 Subject: [PATCH 09/13] Use no-op provider when global provider is null. Signed-off-by: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> --- src/OpenFeatureSDK/OpenFeatureClient.cs | 7 +++++++ test/OpenFeatureSDK.Tests/OpenFeatureClientTests.cs | 8 ++++++++ 2 files changed, 15 insertions(+) diff --git a/src/OpenFeatureSDK/OpenFeatureClient.cs b/src/OpenFeatureSDK/OpenFeatureClient.cs index fef01fe4..5609dc43 100644 --- a/src/OpenFeatureSDK/OpenFeatureClient.cs +++ b/src/OpenFeatureSDK/OpenFeatureClient.cs @@ -41,6 +41,12 @@ public sealed class FeatureClient : IFeatureClient // Alias the provider reference so getting the method and returning the provider are // guaranteed to be the same object. var provider = OpenFeature.Instance.GetProvider(); + + if (provider == null) + { + provider = new NoOpFeatureProvider(); + this._logger.LogDebug("No provider configured, using no-op provider"); + } return (method(provider), provider); } @@ -242,6 +248,7 @@ private async Task> EvaluateFlag( { var resolveValueDelegate = providerInfo.Item1; var provider = providerInfo.Item2; + // New up a evaluation context if one was not provided. if (context == null) { diff --git a/test/OpenFeatureSDK.Tests/OpenFeatureClientTests.cs b/test/OpenFeatureSDK.Tests/OpenFeatureClientTests.cs index 1a13b85b..519fe452 100644 --- a/test/OpenFeatureSDK.Tests/OpenFeatureClientTests.cs +++ b/test/OpenFeatureSDK.Tests/OpenFeatureClientTests.cs @@ -381,6 +381,14 @@ public async Task When_Exception_Occurs_During_Evaluation_Should_Return_Error() featureProviderMock.Verify(x => x.ResolveStructureValue(flagName, defaultValue, It.IsAny()), Times.Once); } + [Fact] + public async Task Should_Use_No_Op_When_Provider_Is_Null() + { + OpenFeature.Instance.SetProvider(null); + var client = new FeatureClient("test", "test"); + (await client.GetIntegerValue("some-key", 12)).Should().Be(12); + } + [Fact] public void Should_Get_And_Set_Context() { From f70d391cf6a6f6a83ac1338c71e15ff861e5d136 Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Tue, 11 Oct 2022 11:16:27 -0700 Subject: [PATCH 10/13] Add documentation on adding hooks. Signed-off-by: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> --- src/OpenFeatureSDK/OpenFeature.cs | 9 ++++++++- src/OpenFeatureSDK/OpenFeatureClient.cs | 8 ++++++++ 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/src/OpenFeatureSDK/OpenFeature.cs b/src/OpenFeatureSDK/OpenFeature.cs index 60c054a1..53b94c84 100644 --- a/src/OpenFeatureSDK/OpenFeature.cs +++ b/src/OpenFeatureSDK/OpenFeature.cs @@ -98,14 +98,21 @@ public FeatureClient GetClient(string name = null, string version = null, ILogge /// /// Appends list of hooks to global hooks list + /// + /// The appending operation will be atomic. + /// /// /// A list of public void AddHooks(IEnumerable hooks) => this._hooks.PushRange(hooks.ToArray()); /// /// Adds a hook to global hooks list + /// + /// Hooks which are dependent on each other should be provided in a collection + /// using the . + /// /// - /// A list of + /// Hook that implements the interface public void AddHooks(Hook hook) => this._hooks.Push(hook); /// diff --git a/src/OpenFeatureSDK/OpenFeatureClient.cs b/src/OpenFeatureSDK/OpenFeatureClient.cs index 5609dc43..78621d67 100644 --- a/src/OpenFeatureSDK/OpenFeatureClient.cs +++ b/src/OpenFeatureSDK/OpenFeatureClient.cs @@ -47,6 +47,7 @@ public sealed class FeatureClient : IFeatureClient provider = new NoOpFeatureProvider(); this._logger.LogDebug("No provider configured, using no-op provider"); } + return (method(provider), provider); } @@ -84,12 +85,19 @@ public FeatureClient(string name, string version, ILogger logger = null, Evaluat /// /// Add hook to client + /// + /// Hooks which are dependent on each other should be provided in a collection + /// using the . + /// /// /// Hook that implements the interface public void AddHooks(Hook hook) => this._hooks.Push(hook); /// /// Appends hooks to client + /// + /// The appending operation will be atomic. + /// /// /// A list of Hooks that implement the interface public void AddHooks(IEnumerable hooks) => this._hooks.PushRange(hooks.ToArray()); From 51cf8f247e730f9cfc9fc56216d4165c576eb055 Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Tue, 11 Oct 2022 15:31:34 -0700 Subject: [PATCH 11/13] Add locking for client context. Signed-off-by: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> --- src/OpenFeatureSDK/OpenFeature.cs | 1 - src/OpenFeatureSDK/OpenFeatureClient.cs | 35 +++++++++++++++++++++++-- 2 files changed, 33 insertions(+), 3 deletions(-) diff --git a/src/OpenFeatureSDK/OpenFeature.cs b/src/OpenFeatureSDK/OpenFeature.cs index 53b94c84..7df24f22 100644 --- a/src/OpenFeatureSDK/OpenFeature.cs +++ b/src/OpenFeatureSDK/OpenFeature.cs @@ -154,7 +154,6 @@ public void SetContext(EvaluationContext context) /// The evaluation context may be set from multiple threads, when accessing the global evaluation context /// it should be accessed once for an operation, and then that reference should be used for all dependent /// operations. - /// . /// /// /// An diff --git a/src/OpenFeatureSDK/OpenFeatureClient.cs b/src/OpenFeatureSDK/OpenFeatureClient.cs index 78621d67..e2aecee9 100644 --- a/src/OpenFeatureSDK/OpenFeatureClient.cs +++ b/src/OpenFeatureSDK/OpenFeatureClient.cs @@ -2,6 +2,7 @@ using System.Collections.Concurrent; using System.Collections.Generic; using System.Linq; +using System.Threading; using System.Threading.Tasks; using Microsoft.Extensions.Logging; using Microsoft.Extensions.Logging.Abstractions; @@ -22,6 +23,8 @@ public sealed class FeatureClient : IFeatureClient private readonly ILogger _logger; private EvaluationContext _evaluationContext; + private readonly ReaderWriterLockSlim _evaluationContextLock = new ReaderWriterLockSlim(); + /// /// Get a provider and an associated typed flag resolution method. /// @@ -53,14 +56,42 @@ public sealed class FeatureClient : IFeatureClient /// /// Gets the EvaluationContext of this client + /// + /// The evaluation context may be set from multiple threads, when accessing the client evaluation context + /// it should be accessed once for an operation, and then that reference should be used for all dependent + /// operations. + /// /// /// of this client - public EvaluationContext GetContext() => this._evaluationContext; + public EvaluationContext GetContext() + { + this._evaluationContextLock.EnterReadLock(); + try + { + return this._evaluationContext; + } + finally + { + this._evaluationContextLock.ExitReadLock(); + } + } /// /// Sets the EvaluationContext of the client /// - public void SetContext(EvaluationContext evaluationContext) => this._evaluationContext = evaluationContext; + /// The to set + public void SetContext(EvaluationContext context) + { + this._evaluationContextLock.EnterWriteLock(); + try + { + this._evaluationContext = context ?? EvaluationContext.Empty; + } + finally + { + this._evaluationContextLock.ExitWriteLock(); + } + } /// /// Initializes a new instance of the class. From b6658b65c5f87eed59e232ad62b00c2ee5c02c68 Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Tue, 11 Oct 2022 15:39:40 -0700 Subject: [PATCH 12/13] Use normal locks for the client level. Signed-off-by: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> --- src/OpenFeatureSDK/OpenFeature.cs | 1 + src/OpenFeatureSDK/OpenFeatureClient.cs | 17 +++-------------- 2 files changed, 4 insertions(+), 14 deletions(-) diff --git a/src/OpenFeatureSDK/OpenFeature.cs b/src/OpenFeatureSDK/OpenFeature.cs index 7df24f22..2664cfd0 100644 --- a/src/OpenFeatureSDK/OpenFeature.cs +++ b/src/OpenFeatureSDK/OpenFeature.cs @@ -1,3 +1,4 @@ +using System; using System.Collections.Concurrent; using System.Collections.Generic; using System.Linq; diff --git a/src/OpenFeatureSDK/OpenFeatureClient.cs b/src/OpenFeatureSDK/OpenFeatureClient.cs index e2aecee9..efe47cb6 100644 --- a/src/OpenFeatureSDK/OpenFeatureClient.cs +++ b/src/OpenFeatureSDK/OpenFeatureClient.cs @@ -2,7 +2,6 @@ using System.Collections.Concurrent; using System.Collections.Generic; using System.Linq; -using System.Threading; using System.Threading.Tasks; using Microsoft.Extensions.Logging; using Microsoft.Extensions.Logging.Abstractions; @@ -23,7 +22,7 @@ public sealed class FeatureClient : IFeatureClient private readonly ILogger _logger; private EvaluationContext _evaluationContext; - private readonly ReaderWriterLockSlim _evaluationContextLock = new ReaderWriterLockSlim(); + private readonly object _evaluationContextLock = new object(); /// /// Get a provider and an associated typed flag resolution method. @@ -65,15 +64,10 @@ public sealed class FeatureClient : IFeatureClient /// of this client public EvaluationContext GetContext() { - this._evaluationContextLock.EnterReadLock(); - try + lock (this._evaluationContextLock) { return this._evaluationContext; } - finally - { - this._evaluationContextLock.ExitReadLock(); - } } /// @@ -82,15 +76,10 @@ public EvaluationContext GetContext() /// The to set public void SetContext(EvaluationContext context) { - this._evaluationContextLock.EnterWriteLock(); - try + lock (this._evaluationContextLock) { this._evaluationContext = context ?? EvaluationContext.Empty; } - finally - { - this._evaluationContextLock.ExitWriteLock(); - } } /// From 094f3d227f884a1f1353db69d30a6295c918f856 Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Tue, 11 Oct 2022 15:42:25 -0700 Subject: [PATCH 13/13] Add comment about disposing. Signed-off-by: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> --- src/OpenFeatureSDK/OpenFeature.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/OpenFeatureSDK/OpenFeature.cs b/src/OpenFeatureSDK/OpenFeature.cs index 2664cfd0..cf119f9f 100644 --- a/src/OpenFeatureSDK/OpenFeature.cs +++ b/src/OpenFeatureSDK/OpenFeature.cs @@ -19,6 +19,7 @@ public sealed class OpenFeature private FeatureProvider _featureProvider = new NoOpFeatureProvider(); private readonly ConcurrentStack _hooks = new ConcurrentStack(); + /// The reader/writer locks are not disposed because the singleton instance should never be disposed. private readonly ReaderWriterLockSlim _evaluationContextLock = new ReaderWriterLockSlim(); private readonly ReaderWriterLockSlim _featureProviderLock = new ReaderWriterLockSlim();