From: Eric Erhardt Date: Tue, 17 Aug 2021 18:13:13 +0000 (-0500) Subject: Revert "Change IOptionsSnapshot to reuse options instances across scopes (#56271... X-Git-Tag: accepted/tizen/unified/20220110.054933~317 X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=f21b2e3f85c73c125344e4536450b9b4f6d67d7e;p=platform%2Fupstream%2Fdotnet%2Fruntime.git Revert "Change IOptionsSnapshot to reuse options instances across scopes (#56271)" (#57570) * Revert "Change IOptionsSnapshot to reuse options instances across scopes (#56271)" This reverts commit 8f5f9d049a6a98b138f88fa1d9d6a96c40c03aa7. * Add a test to ensure ASP.NET's scenario isn't broken again --- diff --git a/src/libraries/Microsoft.Extensions.Options/src/OptionsServiceCollectionExtensions.cs b/src/libraries/Microsoft.Extensions.Options/src/OptionsServiceCollectionExtensions.cs index 3d89c86..a84cbe1 100644 --- a/src/libraries/Microsoft.Extensions.Options/src/OptionsServiceCollectionExtensions.cs +++ b/src/libraries/Microsoft.Extensions.Options/src/OptionsServiceCollectionExtensions.cs @@ -27,7 +27,7 @@ namespace Microsoft.Extensions.DependencyInjection } 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 197d97c..0000000 --- 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 2839069..534700e 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; using System.Linq; using Microsoft.Extensions.Configuration; using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.DependencyInjection.Extensions; using Microsoft.Extensions.Primitives; using Xunit; @@ -131,14 +132,12 @@ namespace Microsoft.Extensions.Options.Tests [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 +149,18 @@ namespace Microsoft.Extensions.Options.Tests 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] @@ -213,5 +200,47 @@ namespace Microsoft.Extensions.Options.Tests { 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) { } + } } }