Skip to content
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.

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

Already on GitHub? Sign in to your account

Change IOptionsSnapshot to reuse options instances across scopes #56271

Merged
merged 4 commits into from
Aug 6, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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(OptionsManager<>)));
services.TryAdd(ServiceDescriptor.Scoped(typeof(IOptionsSnapshot<>), typeof(OptionsSnapshot<>)));
services.TryAdd(ServiceDescriptor.Singleton(typeof(IOptionsMonitor<>), typeof(OptionsMonitor<>)));
services.TryAdd(ServiceDescriptor.Transient(typeof(IOptionsFactory<>), typeof(OptionsFactory<>)));
services.TryAdd(ServiceDescriptor.Singleton(typeof(IOptionsMonitorCache<>), typeof(OptionsCache<>)));
Expand Down
66 changes: 66 additions & 0 deletions src/libraries/Microsoft.Extensions.Options/src/OptionsSnapshot.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
// 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
{
/// <summary>
/// Implementation of <see cref="IOptionsSnapshot{TOptions}"/>.
/// </summary>
/// <typeparam name="TOptions">Options type.</typeparam>
internal sealed class OptionsSnapshot<[DynamicallyAccessedMembers(Options.DynamicallyAccessedMembers)] TOptions> :
IOptionsSnapshot<TOptions>
where TOptions : class
{
private readonly IOptionsMonitor<TOptions> _optionsMonitor;

private volatile ConcurrentDictionary<string, TOptions> _cache;
private volatile TOptions _unnamedOptionsValue;

/// <summary>
/// Initializes a new instance with the specified options configurations.
/// </summary>
/// <param name="optionsMonitor">The options monitor to use to provide options.</param>
public OptionsSnapshot(IOptionsMonitor<TOptions> optionsMonitor)
{
_optionsMonitor = optionsMonitor;
}

/// <summary>
/// The default configured <typeparamref name="TOptions"/> instance, equivalent to Get(Options.DefaultName).
/// </summary>
public TOptions Value => Get(Options.DefaultName);

/// <summary>
/// Returns a configured <typeparamref name="TOptions"/> instance with the given <paramref name="name"/>.
/// </summary>
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;
Copy link
Member

Choose a reason for hiding this comment

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

@stephentoub can you look here? I think it's copied from the other pattern we use in Options<T>

Copy link
Member

Choose a reason for hiding this comment

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

The pattern looks fine. It's saying:

  • Volatile read _cache: if it's not null, use it.
  • It's null, so create a new one and atomically set _cache to a new instance iff _cache is still null.
  • If it wasn't still null, use what it was instead.
  • If it was still null, it's now instead the new instance we set it to, so use that.

Copy link
Member

Choose a reason for hiding this comment

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

I'm waiting for your book to come out. Concurrency patterns with @stephentoub


#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;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -131,12 +131,14 @@ public void Configure(string name, FakeOptions options)
[Fact]
public void SnapshotOptionsAreCachedPerScope()
{
var config = new ConfigurationBuilder().AddInMemoryCollection().Build();
var services = new ServiceCollection()
.AddOptions()
.AddScoped<IConfigureOptions<FakeOptions>, TestConfigure>()
.AddSingleton<IConfigureOptions<FakeOptions>, TestConfigure>()
.AddSingleton<IOptionsChangeTokenSource<FakeOptions>>(new ConfigurationChangeTokenSource<FakeOptions>(Options.DefaultName, config))
.AddSingleton<IOptionsChangeTokenSource<FakeOptions>>(new ConfigurationChangeTokenSource<FakeOptions>("1", config))
.BuildServiceProvider();

var cache = services.GetRequiredService<IOptionsMonitorCache<FakeOptions>>();
var factory = services.GetRequiredService<IServiceScopeFactory>();
FakeOptions options = null;
FakeOptions namedOne = null;
Expand All @@ -148,18 +150,30 @@ public void SnapshotOptionsAreCachedPerScope()
namedOne = scope.ServiceProvider.GetRequiredService<IOptionsSnapshot<FakeOptions>>().Get("1");
Assert.Equal(namedOne, scope.ServiceProvider.GetRequiredService<IOptionsSnapshot<FakeOptions>>().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<IOptionsSnapshot<FakeOptions>>().Value;
Assert.Equal(options, options2);
var namedOne2 = scope.ServiceProvider.GetRequiredService<IOptionsSnapshot<FakeOptions>>().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<IOptionsSnapshot<FakeOptions>>().Value;
Assert.NotEqual(options, options2);
Assert.Equal(3, TestConfigure.ConfigureCount);
NinoFloris marked this conversation as resolved.
Show resolved Hide resolved
Assert.Equal(4, TestConfigure.ConfigureCount);
var namedOne2 = scope.ServiceProvider.GetRequiredService<IOptionsSnapshot<FakeOptions>>().Get("1");
Assert.NotEqual(namedOne2, namedOne);
Assert.NotEqual(namedOne, namedOne2);
Assert.Equal(4, TestConfigure.ConfigureCount);
}
Assert.Equal(2, TestConfigure.CtorCount);
Assert.Equal(1, TestConfigure.CtorCount);
}

[Fact]
Expand Down