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.

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom

Remove Configuration.AutoUpdate and NotifySourcesChanged() #34051

Closed
wants to merge 5 commits into from
Closed

Remove Configuration.AutoUpdate and NotifySourcesChanged() #34051

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a3e349b
Remove Configuration.AutoUpdate and NotifySourcesChanged()
halter73 Jun 30, 2021
44e0030
Reload all sources on FileProvider change
halter73 Jul 3, 2021
007986d
Reload sources on all Properties changes
halter73 Jul 8, 2021
64c26b5
Shorter method names
halter73 Jul 8, 2021
46f9578
Configuration -> Config
halter73 Jul 8, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions src/DefaultBuilder/src/BootstrapHostBuilder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,15 +95,13 @@ internal void RunConfigurationCallbacks()

// Configuration doesn't auto-update during the bootstrap phase to reduce I/O,
// but we do need to update between host and app configuration so the right environment is used.
_configuration.Update();
_environment.ApplyConfigurationSettings(_configuration);

foreach (var configureAppAction in _configureAppActions)
{
configureAppAction(_hostContext, _configuration);
}

_configuration.Update();
_environment.ApplyConfigurationSettings(_configuration);
}
}
Expand Down
213 changes: 140 additions & 73 deletions src/DefaultBuilder/src/Configuration.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
using System;
using System.Collections;
using System.Collections.Generic;
using System.Diagnostics.CodeAnalysis;
using System.Linq;
using System.Threading;
using Microsoft.Extensions.Configuration;
Expand All @@ -19,10 +18,11 @@ namespace Microsoft.AspNetCore.Builder
public sealed class Configuration : IConfigurationRoot, IConfigurationBuilder, IDisposable
Copy link
Member

Choose a reason for hiding this comment

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

Side note: just when I thought we no longer owned configuration :) This seems like a decent candidate for at least inclusion in the extensions configuration packages (if not an easy way to make this the default configuration root), is the idea that we bake this and eventually push this down to extensions.config in a future release?

Copy link
Member

Choose a reason for hiding this comment

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

Plan is to do that for this release

{
private readonly ConfigurationSources _sources;
private ConfigurationRoot _configurationRoot;

private readonly object _providerLock = new();
Copy link
Member Author

Choose a reason for hiding this comment

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

I imagine this lock might be one of the more unexpected/contentious part of this change.

I think we need to lock over all access to _providers and _changeTokenRegistrations but not _sources.

My argument is that nothing should be modifying the IConfigurationBuilder (which is only exposed via explicit interface implementation FWIW) in parallel since that would break with a normal ConfigurationBuilder. However, avoiding reading from the IConfiguration while sources are being added might be harder.

For instance, I would not be surprised if something stored the IConfiguration from the WebHostContext (which very well could be this Configuration type and then attempt to read it later on a background thread while the app is still adding config sources.

Does anyone think I am being too paranoid about _providers? Not paranoid enough about _sources?

Copy link
Member

Choose a reason for hiding this comment

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

Just to make sure I understand what you are saying, we are relying on convention that no one should expect things to behave properly if they modify sources, while we explicitly guard against providers being modified? What's the behavior today, when does the new source become 'live' or visible to everyone? Or is this just not something we need to worry about since we only expose the source APIs for hosting?

Copy link
Member Author

Choose a reason for hiding this comment

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

What's the behavior today, when does the new source become 'live' or visible to everyone?

Today you have to call IConfigurationBuilder.Build() to get the IConfiguration object to read from, so once you get an IConfiguration instance, the sources and providers are static.

This new Configuration type implements both IConfigurationBuilder and IConfiguration at the same time, and the IConfiguration updates without any explicit call to something like Build() or NotifySourcesChanged().

At least theoretically, something could be reading from the Configuration while the sources/providers are being mutated. We could try to say that's not allowed, but with all the layers, this might be a hard requirement to expect people to follow.

Copy link
Member

@HaoK HaoK Jul 6, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Right, that was the idea behind sources/providers to make it clear that its immutable, sorry I wasn't clear, when I meant 'today', I was asking what happens if you add a source while someone is reading it in the current implementation in this PR, will the readers start seeing updated (and possibly inconsistent) config values immediately? I recall there were bugs/weirdness in the past, where readers would get empty config due to errors when reloading config

Copy link
Member

Choose a reason for hiding this comment

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

This is one of the issues we found where the config changed during binding: dotnet/extensions#1202

Copy link
Member Author

@halter73 halter73 Jul 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I think I understand what you're getting out now @HaoK. Today, the IConfigurationRoot is backed by a new ConfigurationRoot instance each time it's updated, so config should be consistent for any given access, though it does look like there's likely a race with Dispose.

Of course, if multiple keys are accessed independently concurrently with a config source being added, that might lead to inconsistent data being read. This could be mitigated by considering the data invalid until you can read all the data without the reload token firing.

private readonly List<IConfigurationProvider> _providers = new();
private readonly List<IDisposable> _changeTokenRegistrations = new();
private ConfigurationReloadToken _changeToken = new();
private IDisposable? _changeTokenRegistration;

/// <summary>
/// Creates an empty mutable configuration object that is both an <see cref="IConfigurationBuilder"/> and an <see cref="IConfigurationRoot"/>.
Expand All @@ -34,54 +34,87 @@ public Configuration()
// Make sure there's some default storage since there are no default providers.
this.AddInMemoryCollection();

Update();
NotifySourceAdded(_sources[0]);
}

/// <summary>
/// Automatically update the <see cref="IConfiguration"/> on <see cref="IConfigurationBuilder"/> changes.
/// If <see langword="false"/>, <see cref="Update()"/> will manually update the <see cref="IConfiguration"/>.
/// </summary>
internal bool AutoUpdate { get; set; } = true;
/// <inheritdoc/>
public string? this[string key]
{
get
{
lock (_providerLock)
{
for (int i = _providers.Count - 1; i >= 0; i--)
Copy link
Member

Choose a reason for hiding this comment

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

The implementation of the indexer should be identical to ConfigurationRoot right? If its too complicated to have a good way to enforce (maybe a new ConfigurationProviderList in extensions?), perhaps put a comment pointing to ConfigurationRoot saying this should stay in sync for posterity?

{
var provider = _providers[i];

/// <inheritdoc />
public string this[string key] { get => _configurationRoot[key]; set => _configurationRoot[key] = value; }
if (provider.TryGet(key, out string value))
{
return value;
}
}

return null;
}
}
set
{
lock (_providerLock)
{
if (_providers.Count == 0)
{
throw new InvalidOperationException("A configuration source is not registered. Please register one before setting a value.");
}

foreach (var provider in _providers)
{
provider.Set(key, value);
}
}
}
}

/// <inheritdoc />
/// <inheritdoc/>
public IConfigurationSection GetSection(string key) => new ConfigurationSection(this, key);

/// <inheritdoc />
public IEnumerable<IConfigurationSection> GetChildren() => GetChildrenImplementation(null);
/// <inheritdoc/>
public IEnumerable<IConfigurationSection> GetChildren()
{
lock (_providerLock)
{
// ToList() to eagerly evaluate inside lock.
return _providers
.Aggregate(Enumerable.Empty<string>(),
static (seed, source) => source.GetChildKeys(seed, parentPath: null))
.Distinct(StringComparer.OrdinalIgnoreCase)
.Select(GetSection)
.ToList();
}
}

IDictionary<string, object> IConfigurationBuilder.Properties { get; } = new Dictionary<string, object>();

IList<IConfigurationSource> IConfigurationBuilder.Sources => _sources;

IEnumerable<IConfigurationProvider> IConfigurationRoot.Providers => _configurationRoot.Providers;

/// <summary>
/// Manually update the <see cref="IConfiguration"/> to reflect <see cref="IConfigurationBuilder"/> changes.
/// It is not necessary to call this if <see cref="AutoUpdate"/> is <see langword="true"/>.
/// </summary>
[MemberNotNull(nameof(_configurationRoot))]
internal void Update()
/// <inheritdoc/>
IEnumerable<IConfigurationProvider> IConfigurationRoot.Providers
{
var newConfiguration = BuildConfigurationRoot();
var prevConfiguration = _configurationRoot;

_configurationRoot = newConfiguration;

_changeTokenRegistration?.Dispose();
(prevConfiguration as IDisposable)?.Dispose();

_changeTokenRegistration = ChangeToken.OnChange(() => newConfiguration.GetReloadToken(), RaiseChanged);
RaiseChanged();
get
{
lock (_providerLock)
{
return new List<IConfigurationProvider>(_providers);
}
}
}

/// <inheritdoc />
void IDisposable.Dispose()
/// <inheritdoc/>
public void Dispose()
{
_changeTokenRegistration?.Dispose();
_configurationRoot?.Dispose();
lock (_providerLock)
{
DisposeRegistrationsAndProvidersUnsynchronized();
}
}

IConfigurationBuilder IConfigurationBuilder.Add(IConfigurationSource source)
Expand All @@ -90,50 +123,84 @@ IConfigurationBuilder IConfigurationBuilder.Add(IConfigurationSource source)
return this;
}

IConfigurationRoot IConfigurationBuilder.Build() => BuildConfigurationRoot();
IConfigurationRoot IConfigurationBuilder.Build() => this;

IChangeToken IConfiguration.GetReloadToken() => _changeToken;

void IConfigurationRoot.Reload() => _configurationRoot.Reload();

private void NotifySourcesChanged()
void IConfigurationRoot.Reload()
{
if (AutoUpdate)
lock (_providerLock)
{
Update();
foreach (var provider in _providers)
{
provider.Load();
}
}

RaiseChanged();
}

private ConfigurationRoot BuildConfigurationRoot()
private void RaiseChanged()
{
var providers = new List<IConfigurationProvider>();
foreach (var source in _sources)
var previousToken = Interlocked.Exchange(ref _changeToken, new ConfigurationReloadToken());
previousToken.OnReload();
}

// Don't rebuild and reload all providers in the common case when a source is simply added to the IList.
private void NotifySourceAdded(IConfigurationSource source)
{
lock (_providerLock)
{
var provider = source.Build(this);
providers.Add(provider);
_providers.Add(provider);

provider.Load();
_changeTokenRegistrations.Add(ChangeToken.OnChange(() => provider.GetReloadToken(), () => RaiseChanged()));
}
return new ConfigurationRoot(providers);

RaiseChanged();
}

private void RaiseChanged()
// Something other than Add was called on IConfigurationBuilder.Sources.
// This is unusual, so we don't bother optimizing it.
private void NotifySourcesChanged()
{
var previousToken = Interlocked.Exchange(ref _changeToken, new ConfigurationReloadToken());
previousToken.OnReload();
lock (_providerLock)
{
DisposeRegistrationsAndProvidersUnsynchronized();

_changeTokenRegistrations.Clear();
_providers.Clear();

foreach (var source in _sources)
{
_providers.Add(source.Build(this));
}

foreach (var p in _providers)
{
p.Load();
_changeTokenRegistrations.Add(ChangeToken.OnChange(() => p.GetReloadToken(), () => RaiseChanged()));
}
}

RaiseChanged();
}

/// <summary>
/// Gets the immediate children sub-sections of configuration root based on key.
/// </summary>
/// <param name="path">Key of a section of which children to retrieve.</param>
/// <returns>Immediate children sub-sections of section specified by key.</returns>
private IEnumerable<IConfigurationSection> GetChildrenImplementation(string? path)

private void DisposeRegistrationsAndProvidersUnsynchronized()
{
// From https://github.com/dotnet/runtime/blob/01b7e73cd378145264a7cb7a09365b41ed42b240/src/libraries/Microsoft.Extensions.Configuration/src/InternalConfigurationRootExtensions.cs
return _configurationRoot.Providers
.Aggregate(Enumerable.Empty<string>(),
(seed, source) => source.GetChildKeys(seed, path))
.Distinct(StringComparer.OrdinalIgnoreCase)
.Select(key => _configurationRoot.GetSection(path == null ? key : ConfigurationPath.Combine(path, key)));
// dispose change token registrations
foreach (var registration in _changeTokenRegistrations)
{
registration.Dispose();
}

// dispose providers
foreach (var provider in _providers)
{
(provider as IDisposable)?.Dispose();
}
}

private class ConfigurationSources : IList<IConfigurationSource>
Expand All @@ -160,10 +227,10 @@ public IConfigurationSource this[int index]

public bool IsReadOnly => false;

public void Add(IConfigurationSource item)
public void Add(IConfigurationSource source)
{
_sources.Add(item);
_config.NotifySourcesChanged();
_sources.Add(source);
_config.NotifySourceAdded(source);
}

public void Clear()
Expand All @@ -172,9 +239,9 @@ public void Clear()
_config.NotifySourcesChanged();
}

public bool Contains(IConfigurationSource item)
public bool Contains(IConfigurationSource source)
{
return _sources.Contains(item);
return _sources.Contains(source);
}

public void CopyTo(IConfigurationSource[] array, int arrayIndex)
Expand All @@ -187,20 +254,20 @@ public IEnumerator<IConfigurationSource> GetEnumerator()
return _sources.GetEnumerator();
}

public int IndexOf(IConfigurationSource item)
public int IndexOf(IConfigurationSource source)
{
return _sources.IndexOf(item);
return _sources.IndexOf(source);
}

public void Insert(int index, IConfigurationSource item)
public void Insert(int index, IConfigurationSource source)
{
_sources.Insert(index, item);
_sources.Insert(index, source);
_config.NotifySourcesChanged();
}

public bool Remove(IConfigurationSource item)
public bool Remove(IConfigurationSource source)
{
var removed = _sources.Remove(item);
var removed = _sources.Remove(source);
_config.NotifySourcesChanged();
return removed;
}
Expand Down
3 changes: 2 additions & 1 deletion src/DefaultBuilder/src/PublicAPI.Unshipped.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
#nullable enable
Microsoft.AspNetCore.Builder.Configuration
Microsoft.AspNetCore.Builder.Configuration.Configuration() -> void
Microsoft.AspNetCore.Builder.Configuration.Dispose() -> void
Microsoft.AspNetCore.Builder.Configuration.GetChildren() -> System.Collections.Generic.IEnumerable<Microsoft.Extensions.Configuration.IConfigurationSection!>!
Microsoft.AspNetCore.Builder.Configuration.GetSection(string! key) -> Microsoft.Extensions.Configuration.IConfigurationSection!
Microsoft.AspNetCore.Builder.Configuration.this[string! key].get -> string!
Microsoft.AspNetCore.Builder.Configuration.this[string! key].get -> string?
Microsoft.AspNetCore.Builder.Configuration.this[string! key].set -> void
Microsoft.AspNetCore.Builder.ConfigureHostBuilder
Microsoft.AspNetCore.Builder.ConfigureHostBuilder.ConfigureAppConfiguration(System.Action<Microsoft.Extensions.Hosting.HostBuilderContext!, Microsoft.Extensions.Configuration.IConfigurationBuilder!>! configureDelegate) -> Microsoft.Extensions.Hosting.IHostBuilder!
Expand Down
6 changes: 2 additions & 4 deletions src/DefaultBuilder/src/WebApplicationBuilder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,11 +62,9 @@ internal WebApplicationBuilder(Assembly? callingAssembly, string[]? args = null)
// Configuration changes made by ConfigureDefaults(args) were already picked up by the BootstrapHostBuilder,
// so we ignore changes to config until ConfigureDefaults completes.
_deferredHostBuilder.ConfigurationEnabled = true;
// Now that consuming code can start modifying Configuration, we need to automatically rebuild on modification.
// To this point, we've been manually calling Configuration.UpdateConfiguration() only when needed to reduce I/O.
Configuration.AutoUpdate = true;
}


/// <summary>
/// Provides information about the web hosting environment an application is running.
/// </summary>
Expand All @@ -80,7 +78,7 @@ internal WebApplicationBuilder(Assembly? callingAssembly, string[]? args = null)
/// <summary>
/// A collection of configuration providers for the application to compose. This is useful for adding new configuration sources and providers.
/// </summary>
public Configuration Configuration { get; } = new() { AutoUpdate = false };
public Configuration Configuration { get; } = new();

/// <summary>
/// A collection of logging providers for the application to compose. This is useful for adding new logging providers.
Expand Down
Loading