From 31bcfbd52922ec56e38b2f0e959fe75d256a5011 Mon Sep 17 00:00:00 2001 From: Jimmy Campbell Date: Wed, 1 Sep 2021 14:03:51 -0700 Subject: [PATCH 1/3] Make FeatureManagementSnapshot thread safe. --- .../FeatureManagerSnapshot.cs | 40 ++++++++----------- 1 file changed, 17 insertions(+), 23 deletions(-) diff --git a/src/Microsoft.FeatureManagement/FeatureManagerSnapshot.cs b/src/Microsoft.FeatureManagement/FeatureManagerSnapshot.cs index a218e1cc..95bb805d 100644 --- a/src/Microsoft.FeatureManagement/FeatureManagerSnapshot.cs +++ b/src/Microsoft.FeatureManagement/FeatureManagerSnapshot.cs @@ -2,7 +2,9 @@ // Licensed under the MIT license. // using System; +using System.Collections.Concurrent; using System.Collections.Generic; +using System.Threading; using System.Threading.Tasks; namespace Microsoft.FeatureManagement @@ -13,7 +15,7 @@ namespace Microsoft.FeatureManagement class FeatureManagerSnapshot : IFeatureManagerSnapshot { private readonly IFeatureManager _featureManager; - private readonly IDictionary _flagCache = new Dictionary(); + private readonly ConcurrentDictionary>> _flagCache = new ConcurrentDictionary>>(); private IEnumerable _featureNames; public FeatureManagerSnapshot(IFeatureManager featureManager) @@ -43,34 +45,26 @@ public async IAsyncEnumerable GetFeatureNamesAsync() public async Task IsEnabledAsync(string feature) { - // - // First, check local cache - if (_flagCache.ContainsKey(feature)) - { - return _flagCache[feature]; - } - - bool enabled = await _featureManager.IsEnabledAsync(feature).ConfigureAwait(false); - - _flagCache[feature] = enabled; + Lazy> evaluator = _flagCache.GetOrAdd( + feature, + (key) => + new Lazy>( + () => _featureManager.IsEnabledAsync(key), + LazyThreadSafetyMode.ExecutionAndPublication)); - return enabled; + return await evaluator.Value.ConfigureAwait(false); } public async Task IsEnabledAsync(string feature, TContext context) { - // - // First, check local cache - if (_flagCache.ContainsKey(feature)) - { - return _flagCache[feature]; - } - - bool enabled = await _featureManager.IsEnabledAsync(feature, context).ConfigureAwait(false); - - _flagCache[feature] = enabled; + Lazy> evaluator = _flagCache.GetOrAdd( + feature, + (key) => + new Lazy>( + () => _featureManager.IsEnabledAsync(key, context), + LazyThreadSafetyMode.ExecutionAndPublication)); - return enabled; + return await evaluator.Value.ConfigureAwait(false); } } } From 233feefb027bcf889d34dc0587d3c07eaaf19d11 Mon Sep 17 00:00:00 2001 From: Jimmy Campbell Date: Wed, 1 Sep 2021 14:24:47 -0700 Subject: [PATCH 2/3] Add feature manager snapshot concurrency test. --- .../FeatureManagement.cs | 66 +++++++++++++++++-- tests/Tests.FeatureManagement/TestFilter.cs | 4 +- 2 files changed, 61 insertions(+), 9 deletions(-) diff --git a/tests/Tests.FeatureManagement/FeatureManagement.cs b/tests/Tests.FeatureManagement/FeatureManagement.cs index d7e85366..2dade89c 100644 --- a/tests/Tests.FeatureManagement/FeatureManagement.cs +++ b/tests/Tests.FeatureManagement/FeatureManagement.cs @@ -63,7 +63,7 @@ public async Task ReadsConfiguration() Assert.Equal(ConditionalFeature, evaluationContext.FeatureName); - return true; + return Task.FromResult(true); }; await featureManager.IsEnabledAsync(ConditionalFeature); @@ -106,14 +106,14 @@ public async Task Integrates() TestFilter testFeatureFilter = (TestFilter)featureFilters.First(f => f is TestFilter); - testFeatureFilter.Callback = _ => true; + testFeatureFilter.Callback = _ => Task.FromResult(true); HttpResponseMessage res = await testServer.CreateClient().GetAsync(""); Assert.True(res.Headers.Contains(nameof(MvcFilter))); Assert.True(res.Headers.Contains(nameof(RouterMiddleware))); - testFeatureFilter.Callback = _ => false; + testFeatureFilter.Callback = _ => Task.FromResult(false); res = await testServer.CreateClient().GetAsync(""); @@ -143,7 +143,7 @@ public async Task GatesFeatures() // // Enable all features - testFeatureFilter.Callback = ctx => true; + testFeatureFilter.Callback = ctx => Task.FromResult(true); HttpResponseMessage gateAllResponse = await testServer.CreateClient().GetAsync("gateAll"); HttpResponseMessage gateAnyResponse = await testServer.CreateClient().GetAsync("gateAny"); @@ -153,7 +153,7 @@ public async Task GatesFeatures() // // Enable 1/2 features - testFeatureFilter.Callback = ctx => ctx.FeatureName == Enum.GetName(typeof(Features), Features.ConditionalFeature); + testFeatureFilter.Callback = ctx => Task.FromResult(ctx.FeatureName == Enum.GetName(typeof(Features), Features.ConditionalFeature)); gateAllResponse = await testServer.CreateClient().GetAsync("gateAll"); gateAnyResponse = await testServer.CreateClient().GetAsync("gateAny"); @@ -163,7 +163,7 @@ public async Task GatesFeatures() // // Enable no - testFeatureFilter.Callback = ctx => false; + testFeatureFilter.Callback = ctx => Task.FromResult(false); gateAllResponse = await testServer.CreateClient().GetAsync("gateAll"); gateAnyResponse = await testServer.CreateClient().GetAsync("gateAny"); @@ -555,7 +555,7 @@ public async Task CustomFeatureDefinitionProvider() Assert.Equal(ConditionalFeature, evaluationContext.FeatureName); - return true; + return Task.FromResult(true); }; await featureManager.IsEnabledAsync(ConditionalFeature); @@ -563,6 +563,58 @@ public async Task CustomFeatureDefinitionProvider() Assert.True(called); } + [Fact] + public async Task ThreadsafeSnapshot() + { + IConfiguration config = new ConfigurationBuilder().AddJsonFile("appsettings.json").Build(); + + var services = new ServiceCollection(); + + services + .AddSingleton(config) + .AddFeatureManagement() + .AddFeatureFilter(); + + ServiceProvider serviceProvider = services.BuildServiceProvider(); + + IFeatureManager featureManager = serviceProvider.GetRequiredService(); + + IEnumerable featureFilters = serviceProvider.GetRequiredService>(); + + // + // Sync filter + TestFilter testFeatureFilter = (TestFilter)featureFilters.First(f => f is TestFilter); + + bool called = false; + + testFeatureFilter.Callback = async (evaluationContext) => + { + called = true; + + await Task.Delay(10); + + return new Random().Next(0, 100) > 50; + }; + + var tasks = new List>(); + + for (int i = 0; i < 1000; i++) + { + tasks.Add(featureManager.IsEnabledAsync(ConditionalFeature)); + } + + Assert.True(called); + + await Task.WhenAll(tasks); + + bool result = tasks.First().Result; + + foreach (Task t in tasks) + { + Assert.Equal(result, t.Result); + } + } + private static void DisableEndpointRouting(MvcOptions options) { #if NET5_0 || NETCOREAPP3_1 diff --git a/tests/Tests.FeatureManagement/TestFilter.cs b/tests/Tests.FeatureManagement/TestFilter.cs index ed236742..d3db4225 100644 --- a/tests/Tests.FeatureManagement/TestFilter.cs +++ b/tests/Tests.FeatureManagement/TestFilter.cs @@ -9,11 +9,11 @@ namespace Tests.FeatureManagement { class TestFilter : IFeatureFilter { - public Func Callback { get; set; } + public Func> Callback { get; set; } public Task EvaluateAsync(FeatureFilterEvaluationContext context) { - return Task.FromResult(Callback?.Invoke(context) ?? false); + return Callback?.Invoke(context) ?? Task.FromResult(false); } } } From 4465562957ac62fbc2a1c165e2029547f8ce7b25 Mon Sep 17 00:00:00 2001 From: Jimmy Campbell Date: Thu, 2 Sep 2021 16:05:29 -0700 Subject: [PATCH 3/3] Remove lazy usage. --- .../FeatureManagerSnapshot.cs | 25 ++++++------------- 1 file changed, 7 insertions(+), 18 deletions(-) diff --git a/src/Microsoft.FeatureManagement/FeatureManagerSnapshot.cs b/src/Microsoft.FeatureManagement/FeatureManagerSnapshot.cs index 95bb805d..1c676cbb 100644 --- a/src/Microsoft.FeatureManagement/FeatureManagerSnapshot.cs +++ b/src/Microsoft.FeatureManagement/FeatureManagerSnapshot.cs @@ -4,7 +4,6 @@ using System; using System.Collections.Concurrent; using System.Collections.Generic; -using System.Threading; using System.Threading.Tasks; namespace Microsoft.FeatureManagement @@ -15,7 +14,7 @@ namespace Microsoft.FeatureManagement class FeatureManagerSnapshot : IFeatureManagerSnapshot { private readonly IFeatureManager _featureManager; - private readonly ConcurrentDictionary>> _flagCache = new ConcurrentDictionary>>(); + private readonly ConcurrentDictionary> _flagCache = new ConcurrentDictionary>(); private IEnumerable _featureNames; public FeatureManagerSnapshot(IFeatureManager featureManager) @@ -43,28 +42,18 @@ public async IAsyncEnumerable GetFeatureNamesAsync() } } - public async Task IsEnabledAsync(string feature) + public Task IsEnabledAsync(string feature) { - Lazy> evaluator = _flagCache.GetOrAdd( + return _flagCache.GetOrAdd( feature, - (key) => - new Lazy>( - () => _featureManager.IsEnabledAsync(key), - LazyThreadSafetyMode.ExecutionAndPublication)); - - return await evaluator.Value.ConfigureAwait(false); + (key) => _featureManager.IsEnabledAsync(key)); } - public async Task IsEnabledAsync(string feature, TContext context) + public Task IsEnabledAsync(string feature, TContext context) { - Lazy> evaluator = _flagCache.GetOrAdd( + return _flagCache.GetOrAdd( feature, - (key) => - new Lazy>( - () => _featureManager.IsEnabledAsync(key, context), - LazyThreadSafetyMode.ExecutionAndPublication)); - - return await evaluator.Value.ConfigureAwait(false); + (key) => _featureManager.IsEnabledAsync(key, context)); } } }