Skip to content
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
Prev Previous commit
Next Next commit
Reload all sources on FileProvider change
  • Loading branch information
halter73 committed Jul 3, 2021
commit 44e00304be21e2418991eddc9a93735f33f27ffd
112 changes: 109 additions & 3 deletions src/DefaultBuilder/src/Configuration.cs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
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 @@ -18,6 +19,7 @@ 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 readonly ConfigurationBuilderProperties _properties;

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

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

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();
Expand All @@ -30,6 +32,7 @@ public sealed class Configuration : IConfigurationRoot, IConfigurationBuilder, I
public Configuration()
{
_sources = new ConfigurationSources(this);
_properties = new ConfigurationBuilderProperties(this);

// Make sure there's some default storage since there are no default providers.
this.AddInMemoryCollection();
Expand Down Expand Up @@ -92,7 +95,7 @@ public IEnumerable<IConfigurationSection> GetChildren()
}
}

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

IList<IConfigurationSource> IConfigurationBuilder.Sources => _sources;

Expand Down Expand Up @@ -161,7 +164,7 @@ private void NotifySourceAdded(IConfigurationSource source)
RaiseChanged();
}

// Something other than Add was called on IConfigurationBuilder.Sources.
// Something other than Add was called on IConfigurationBuilder.Sources or the FileProvider has been set to a new value.
// This is unusual, so we don't bother optimizing it.
private void NotifySourcesChanged()
{
Expand All @@ -187,7 +190,6 @@ private void NotifySourcesChanged()
RaiseChanged();
}


private void DisposeRegistrationsAndProvidersUnsynchronized()
{
// dispose change token registrations
Expand Down Expand Up @@ -283,5 +285,109 @@ IEnumerator IEnumerable.GetEnumerator()
return GetEnumerator();
}
}

private class ConfigurationBuilderProperties : IDictionary<string, object>
{
private const string FileProviderKey = "FileProvider";

private readonly Dictionary<string, object> _properties = new();
private readonly Configuration _config;
private object? _fileProvider;

public ConfigurationBuilderProperties(Configuration config)
{
_config = config;
}

public object this[string key]
{
get => _properties[key];
set
{
_properties[key] = value;
CheckForFileProviderChange(key, value);
}
}

public ICollection<string> Keys => _properties.Keys;

public ICollection<object> Values => _properties.Values;

public int Count => _properties.Count;

public bool IsReadOnly => false;

public void Add(string key, object value)
{
_properties.Add(key, value);
CheckForFileProviderChange(key, value);
}

public void Add(KeyValuePair<string, object> item)
{
((IDictionary<string, object>)_properties).Add(item);
CheckForFileProviderChange(item.Key, item.Value);
}

public void Clear()
{
_properties.Clear();
CheckForFileProviderChange(FileProviderKey, null);
}

public bool Contains(KeyValuePair<string, object> item)
{
return _properties.Contains(item);
}

public bool ContainsKey(string key)
{
return _properties.ContainsKey(key);
}

public void CopyTo(KeyValuePair<string, object>[] array, int arrayIndex)
{
((IDictionary<string, object>)_properties).CopyTo(array, arrayIndex);
}

public IEnumerator<KeyValuePair<string, object>> GetEnumerator()
{
return _properties.GetEnumerator();
}

public bool Remove(string key)
{
var wasRemoved = _properties.Remove(key);
CheckForFileProviderChange(key, null);
return wasRemoved;
}

public bool Remove(KeyValuePair<string, object> item)
{
var wasRemoved = ((IDictionary<string, object>)_properties).Remove(item);
CheckForFileProviderChange(item.Key, null);
return wasRemoved;
}

public bool TryGetValue(string key, [MaybeNullWhen(false)] out object value)
{
return _properties.TryGetValue(key, out value);
}

IEnumerator IEnumerable.GetEnumerator()
{
return _properties.GetEnumerator();
}

private void CheckForFileProviderChange(string key, object? value)
Copy link
Contributor

Choose a reason for hiding this comment

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

Surprised we special-cased this to the file provider. I thought we would invalidate the configuration on every property change because it's impossible to know how they would affect the resulting configuration.

Copy link
Member Author

@halter73 halter73 Jul 6, 2021

Choose a reason for hiding this comment

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

This isn't even checking for a change to the configuration per se, but IConfigurationBuilder.Properties. This is very likely to change where file-based config provider are loaded from.

As for config effecting where other sources are loaded from, that's certainly possible in a scenario like the one laid out as a "Usage Example" in dotnet/runtime#51770, but I think it works fine without reloading sources as long as you're careful about the order you load sources in.

If in the future we really want this to be able to define config-dependent config sources that react to new config, I think we'd have to add an API like. IDisposable Configuration.RegisterDependentSources(Action<IConfigurationBuilder> dependentConfigCallback).

Ex:

    using var config = new Configuration();

    config.AddEnvironmentVariables(prefix: "MyCustomPrefix_");

    // If MyCustomPrefix_FileConfig = "enabled", this will add config from MyConfig.json
	cofig.RegisterDependentSources(emptyConfigBuilder =>
    {
        if (config["FileConfig"] == "enabled")
        {
            emptyConfigBuilder.AddJsonFile("MyConfig.json", optional: true, reloadOnChange: true);
        }
    });
    
    // Adding more sources or any change token firing causes the RegisterDependentSource to be rerun.
    // If MyCustomPrefix2_FileConfig = "disabled", this will remove config from MyConfig.json even if MyCustomPrefix_FileConfig = "enabled"
    config.AddEnvironmentVariables(prefix: "MyCustomPrefix2_");

    string myValueFromJson = config["JsonConfigValue"];
    // ...

Though that'd still be tricky, because how do you know it's stable? How do you know dependentConfigCallback didn't add a source that would cause it to add a different source the next time? If there are multiple callbacks, do you just run them in order?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nvm. I reread the comment and see you are referring specifically to IConfigurationBuilder.Properties @pakrym. This makes way more sense. I'll update to reload everything on any property change.

{
if (key == FileProviderKey && !ReferenceEquals(value, _fileProvider))
{
_fileProvider = value;
// Reload all the sources since the base path has likely changed.
_config.NotifySourcesChanged();
}
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -1091,7 +1091,6 @@ public void BuildReturnsThis()
Assert.Same(config, ((IConfigurationBuilder)config).Build());
}


private static string Get(IConfigurationProvider provider, string key)
{
string value;
Expand Down