Remove Configuration.AutoUpdate and NotifySourcesChanged()

[halter73]

This (hopefully) properly implements the combined IConfiguration/IConfigurationBuilder type proposed by dotnet/runtime#51770 without the need to have an AutoUpdate property or internal NotifySourcesChanged().

The new implementation essentially AutoUpdates always, so there's no need for a non-private NotifySourcesChanged() method. This relies on the assumption that adding sources is far more common than any other kind of modification of IConfigurationBuilder.Sources. Any other modification triggers a full rebuild like it did before in AutoUpdate = true case.

Fixes #33083

[halter73]

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?

[HaoK]

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?

[halter73]

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.

[HaoK]

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

[martincostello]

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

[halter73]

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.

[HaoK]

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?

[davidfowl]

Plan is to do that for this release

[HaoK]

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?

[pakrym]

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.

[halter73]

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?

[halter73]

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.

[halter73]

Closing in favor of dotnet/runtime#55338 which adds Config. This is the same as the Configuration type modified by this PR, but it could not be named Configuration while moving it to the Microsoft.Extensions.Configuration namespace. The new Config type will be referenced from WebApplicationBuilder and Configuration will be deleted.