From cd5d0b7ece67faaf422c1b72e778441572f7b97b Mon Sep 17 00:00:00 2001 From: Eric Erhardt Date: Tue, 17 Aug 2021 08:47:43 -0500 Subject: [PATCH 1/2] Revert "Change IOptionsSnapshot to reuse options instances across scopes (#56271)" This reverts commit 8f5f9d049a6a98b138f88fa1d9d6a96c40c03aa7. --- .../src/OptionsServiceCollectionExtensions.cs | 2 +- .../src/OptionsSnapshot.cs | 66 ------------------- .../OptionsSnapshotTest.cs | 24 ++----- 3 files changed, 6 insertions(+), 86 deletions(-) delete mode 100644 src/libraries/Microsoft.Extensions.Options/src/OptionsSnapshot.cs diff --git a/src/libraries/Microsoft.Extensions.Options/src/OptionsServiceCollectionExtensions.cs b/src/libraries/Microsoft.Extensions.Options/src/OptionsServiceCollectionExtensions.cs index 3d89c86e2c90ac..a84cbe19e1fa6f 100644 --- a/src/libraries/Microsoft.Extensions.Options/src/OptionsServiceCollectionExtensions.cs +++ b/src/libraries/Microsoft.Extensions.Options/src/OptionsServiceCollectionExtensions.cs @@ -27,7 +27,7 @@ public static IServiceCollection AddOptions(this IServiceCollection services) } services.TryAdd(ServiceDescriptor.Singleton(typeof(IOptions<>), typeof(UnnamedOptionsManager<>))); - services.TryAdd(ServiceDescriptor.Scoped(typeof(IOptionsSnapshot<>), typeof(OptionsSnapshot<>))); + services.TryAdd(ServiceDescriptor.Scoped(typeof(IOptionsSnapshot<>), typeof(OptionsManager<>))); services.TryAdd(ServiceDescriptor.Singleton(typeof(IOptionsMonitor<>), typeof(OptionsMonitor<>))); services.TryAdd(ServiceDescriptor.Transient(typeof(IOptionsFactory<>), typeof(OptionsFactory<>))); services.TryAdd(ServiceDescriptor.Singleton(typeof(IOptionsMonitorCache<>), typeof(OptionsCache<>))); diff --git a/src/libraries/Microsoft.Extensions.Options/src/OptionsSnapshot.cs b/src/libraries/Microsoft.Extensions.Options/src/OptionsSnapshot.cs deleted file mode 100644 index 197d97c19d7e7d..00000000000000 --- a/src/libraries/Microsoft.Extensions.Options/src/OptionsSnapshot.cs +++ /dev/null @@ -1,66 +0,0 @@ -// Licensed to the .NET Foundation under one or more agreements. -// The .NET Foundation licenses this file to you under the MIT license. - -using System; -using System.Collections.Concurrent; -using System.Diagnostics.CodeAnalysis; -using System.Threading; - -namespace Microsoft.Extensions.Options -{ - /// - /// Implementation of . - /// - /// Options type. - internal sealed class OptionsSnapshot<[DynamicallyAccessedMembers(Options.DynamicallyAccessedMembers)] TOptions> : - IOptionsSnapshot - where TOptions : class - { - private readonly IOptionsMonitor _optionsMonitor; - - private volatile ConcurrentDictionary _cache; - private volatile TOptions _unnamedOptionsValue; - - /// - /// Initializes a new instance with the specified options configurations. - /// - /// The options monitor to use to provide options. - public OptionsSnapshot(IOptionsMonitor optionsMonitor) - { - _optionsMonitor = optionsMonitor; - } - - /// - /// The default configured instance, equivalent to Get(Options.DefaultName). - /// - public TOptions Value => Get(Options.DefaultName); - - /// - /// Returns a configured instance with the given . - /// - public TOptions Get(string name) - { - if (name == null || name == Options.DefaultName) - { - if (_unnamedOptionsValue is TOptions value) - { - return value; - } - - return _unnamedOptionsValue = _optionsMonitor.Get(Options.DefaultName); - } - - var cache = _cache ?? Interlocked.CompareExchange(ref _cache, new(concurrencyLevel: 1, capacity: 5, StringComparer.Ordinal), null) ?? _cache; - -#if NETSTANDARD2_1 - TOptions options = cache.GetOrAdd(name, static (name, optionsMonitor) => optionsMonitor.Get(name), _optionsMonitor); -#else - if (!cache.TryGetValue(name, out TOptions options)) - { - options = cache.GetOrAdd(name, _optionsMonitor.Get(name)); - } -#endif - return options; - } - } -} diff --git a/src/libraries/Microsoft.Extensions.Options/tests/Microsoft.Extensions.Options.Tests/OptionsSnapshotTest.cs b/src/libraries/Microsoft.Extensions.Options/tests/Microsoft.Extensions.Options.Tests/OptionsSnapshotTest.cs index 28390691c6903e..adb63a5c6913c9 100644 --- a/src/libraries/Microsoft.Extensions.Options/tests/Microsoft.Extensions.Options.Tests/OptionsSnapshotTest.cs +++ b/src/libraries/Microsoft.Extensions.Options/tests/Microsoft.Extensions.Options.Tests/OptionsSnapshotTest.cs @@ -131,14 +131,12 @@ public void Configure(string name, FakeOptions options) [Fact] public void SnapshotOptionsAreCachedPerScope() { - var config = new ConfigurationBuilder().AddInMemoryCollection().Build(); var services = new ServiceCollection() .AddOptions() - .AddSingleton, TestConfigure>() - .AddSingleton>(new ConfigurationChangeTokenSource(Options.DefaultName, config)) - .AddSingleton>(new ConfigurationChangeTokenSource("1", config)) + .AddScoped, TestConfigure>() .BuildServiceProvider(); + var cache = services.GetRequiredService>(); var factory = services.GetRequiredService(); FakeOptions options = null; FakeOptions namedOne = null; @@ -150,30 +148,18 @@ public void SnapshotOptionsAreCachedPerScope() namedOne = scope.ServiceProvider.GetRequiredService>().Get("1"); Assert.Equal(namedOne, scope.ServiceProvider.GetRequiredService>().Get("1")); Assert.Equal(2, TestConfigure.ConfigureCount); - - // Reload triggers Configure for the two registered change tokens. - config.Reload(); - Assert.Equal(4, TestConfigure.ConfigureCount); - - // Reload should not affect current scope. - var options2 = scope.ServiceProvider.GetRequiredService>().Value; - Assert.Equal(options, options2); - var namedOne2 = scope.ServiceProvider.GetRequiredService>().Get("1"); - Assert.Equal(namedOne, namedOne2); } Assert.Equal(1, TestConfigure.CtorCount); - - // Reload should be reflected in a fresh scope. using (var scope = factory.CreateScope()) { var options2 = scope.ServiceProvider.GetRequiredService>().Value; Assert.NotEqual(options, options2); - Assert.Equal(4, TestConfigure.ConfigureCount); + Assert.Equal(3, TestConfigure.ConfigureCount); var namedOne2 = scope.ServiceProvider.GetRequiredService>().Get("1"); - Assert.NotEqual(namedOne, namedOne2); + Assert.NotEqual(namedOne2, namedOne); Assert.Equal(4, TestConfigure.ConfigureCount); } - Assert.Equal(1, TestConfigure.CtorCount); + Assert.Equal(2, TestConfigure.CtorCount); } [Fact] From 835243341681a77c81eae5fc39562239d29d3fdc Mon Sep 17 00:00:00 2001 From: Eric Erhardt Date: Tue, 17 Aug 2021 09:19:10 -0500 Subject: [PATCH 2/2] Add a test to ensure ASP.NET's scenario isn't broken again --- .../OptionsSnapshotTest.cs | 43 +++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/src/libraries/Microsoft.Extensions.Options/tests/Microsoft.Extensions.Options.Tests/OptionsSnapshotTest.cs b/src/libraries/Microsoft.Extensions.Options/tests/Microsoft.Extensions.Options.Tests/OptionsSnapshotTest.cs index adb63a5c6913c9..534700e763ad3a 100644 --- a/src/libraries/Microsoft.Extensions.Options/tests/Microsoft.Extensions.Options.Tests/OptionsSnapshotTest.cs +++ b/src/libraries/Microsoft.Extensions.Options/tests/Microsoft.Extensions.Options.Tests/OptionsSnapshotTest.cs @@ -5,6 +5,7 @@ using System.Linq; using Microsoft.Extensions.Configuration; using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.DependencyInjection.Extensions; using Microsoft.Extensions.Primitives; using Xunit; @@ -199,5 +200,47 @@ private void CheckLifetime(IServiceCollection services, Type serviceType, Servic { Assert.NotNull(services.Where(s => s.ServiceType == serviceType && s.Lifetime == lifetime).SingleOrDefault()); } + + /// + /// Duplicates an aspnetcore test to ensure when an IOptionsSnapshot is resolved both in + /// the root scope and a created scope, that dependent services are created both times. + /// + [Fact] + public void RecreateAspNetCore_AddOidc_CustomStateAndAccount_SetsUpConfiguration() + { + var services = new ServiceCollection().AddOptions(); + + int calls = 0; + + services.TryAddEnumerable(ServiceDescriptor.Scoped>, DefaultOidcOptionsConfiguration>()); + services.Replace(ServiceDescriptor.Scoped(typeof(NavigationManager), _ => + { + calls++; + return new NavigationManager(); + })); + + using ServiceProvider provider = services.BuildServiceProvider(); + + using IServiceScope scope = provider.CreateScope(); + + // from the root scope. + var rootOptions = provider.GetRequiredService>>(); + + // from the created scope + var scopedOptions = scope.ServiceProvider.GetRequiredService>>(); + + // we should have 2 navigation managers. One in the root scope, and one in the created scope. + Assert.Equal(2, calls); + } + + private class OidcProviderOptions { } + private class RemoteAuthenticationOptions where TRemoteAuthenticationProviderOptions : new() { } + private class NavigationManager { } + + private class DefaultOidcOptionsConfiguration : IPostConfigureOptions> + { + public DefaultOidcOptionsConfiguration(NavigationManager navigationManager) { } + public void PostConfigure(string name, RemoteAuthenticationOptions options) { } + } } }