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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[API Proposal]: AsyncConfigurationProvider #79193

Open
KieranDevvs opened this issue Dec 3, 2022 · 19 comments
Open

[API Proposal]: AsyncConfigurationProvider #79193

KieranDevvs opened this issue Dec 3, 2022 · 19 comments
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-Extensions-Configuration
Milestone

Comments

@KieranDevvs
Copy link

KieranDevvs commented Dec 3, 2022

Background and motivation

As it currently stands, configuration sources have no way of being asynchronous (Highlighted in #36018).
These new set of types should allow for users to implement asynchronous configuration providers with minimal breaking changes to the way the existing API's function.

API Proposal

namespace Microsoft.Extensions.Configuration;

public partial class ConfigurationBuilder : IConfigurationBuilder
{
+    public IConfigurationBuilder Add(IAsyncConfigurationSource source);
+    public Task<IConfigurationRoot> BuildAsync();
}
namespace Microsoft.Extensions.Configuration;

public interface IAsyncConfigurationSource
{
    IAsyncConfigurationProvider Build(IConfigurationBuilder builder);
}
namespace Microsoft.Extensions.Configuration;

public interface IAsyncConfigurationProvider
{
    bool TryGet(string key, out string? value);
    void Set(string key, string? value);
    IChangeToken GetReloadToken();
    Task LoadAsync();
    IEnumerable<string> GetChildKeys(IEnumerable<string> earlierKeys, string? parentPath);
}
namespace Microsoft.Extensions.Configuration;

public abstract class AsyncConfigurationProvider : IAsyncConfigurationProvider
{
    protected IDictionary<string, string?> Data { get; set; }
    public virtual bool TryGet(string key, out string? value);
    public virtual void Set(string key, string? value);
    public virtual Task LoadAsync();
    public virtual IEnumerable<string> GetChildKeys(IEnumerable<string> earlierKeys, string? parentPath);
    public IChangeToken GetReloadToken();
    protected void OnReload()
}

API Usage

ConfigurationBuilder.Build() would call Load() for all IConfigurationProvider's, if async providers are registered, an exception is thrown.

ConfigurationBuilder.BuildAsync() would call Load() for all IConfigurationProvider's and await LoadAsync() for all IAsyncConfigurationProvider's

internal class Program
{
    static async Task Main(string[] args)
    {
        var builder = new ConfigurationBuilder();
        builder.Add(new DatabaseConfigurationSource());
        _ = await builder.BuildAsync();
    }
}

public class DatabaseConfigurationSource : IAsyncConfigurationSource
{
    public IAsyncConfigurationProvider Build(IConfigurationBuilder builder)
    {
        return new DatabaseConfigurationProvider();
    }
}

public class DatabaseConfigurationProvider : AsyncConfigurationProvider
{
    public override async Task LoadAync()
    {
        Data = await ...
    }
}

Alternative Designs

It might be possible to add an async method into the existing type but I'm not sure how the implementation would know to use the asynchronous call.

namespace Microsoft.Extensions.Configuration

public abstract partial class ConfigurationProvider : IConfigurationProvider
{
+    public virtual Task LoadAsync();
}

Risks

Usage of the new types would require users to call and await ConfigurationBuilder.BuildAsync() rather than ConfigurationBuilder.Build().

@KieranDevvs KieranDevvs added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Dec 3, 2022
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Dec 3, 2022
@ghost
Copy link

ghost commented Dec 3, 2022

Tagging subscribers to this area: @dotnet/area-extensions-configuration
See info in area-owners.md if you want to be subscribed.

Issue Details

Background and motivation

As it currently stands, configuration sources have no way of being asynchronous (Highlighted in #36018).
These new set of types should allow for users to implement asynchronous configuration providers without any breaking changes to the way the existing API's function.

API Proposal

namespace Microsoft.Extensions.Configuration;

public partial class ConfigurationBuilder : IConfigurationBuilder
{
+    public IConfigurationBuilder Add(IAsyncConfigurationSource source);
}
namespace Microsoft.Extensions.Configuration;

public interface IAsyncConfigurationSource
{
    IAsyncConfigurationProvider Build(IConfigurationBuilder builder);
}
namespace Microsoft.Extensions.Configuration;

public interface IAsyncConfigurationProvider
{
    bool TryGet(string key, out string? value);
    void Set(string key, string? value);
    IChangeToken GetReloadToken();
    Task LoadAsync();
    IEnumerable<string> GetChildKeys(IEnumerable<string> earlierKeys, string? parentPath);
}
namespace Microsoft.Extensions.Configuration;

public abstract class AsyncConfigurationProvider : IAsyncConfigurationProvider
{
    protected IDictionary<string, string?> Data { get; set; }
    public virtual bool TryGet(string key, out string? value);
    public virtual void Set(string key, string? value);
    public virtual Task LoadAsync();
    public virtual IEnumerable<string> GetChildKeys(IEnumerable<string> earlierKeys, string? parentPath);
    public IChangeToken GetReloadToken();
    protected void OnReload()
}

API Usage

internal class Program
{
    static void Main(string[] args)
    {
        var builder = new ConfigurationBuilder();
        builder.Add(new DatabaseConfigurationSource());
    }
}

public class DatabaseConfigurationSource : IAsyncConfigurationSource
{
    public IAsyncConfigurationProvider Build(IConfigurationBuilder builder)
    {
        return new DatabaseConfigurationProvider ();
    }
}

public class DatabaseConfigurationProvider : AsyncConfigurationProvider
{
    public override async Task LoadAync()
    {
        Data = await ...
    }
}

Alternative Designs

It might be possible to add an async method into the existing type but I'm not sure how the implementation would know to use the asynchronous call.

namespace Microsoft.Extensions.Configuration

public abstract partial class ConfigurationProvider : IConfigurationProvider
{
+    public virtual Task LoadAsync();
}

Risks

No response

Author: KieranDevvs
Assignees: -
Labels:

api-suggestion, area-Extensions-Configuration

Milestone: -

@davidfowl
Copy link
Member

davidfowl commented Dec 3, 2022

The alternative design looks better but did you also look at all of the consumers of the Load method to understand what else would need to be changed to async to make this work in practice? What about the generic hosting APIs?

@tarekgh tarekgh added this to the Future milestone Dec 4, 2022
@ghost ghost removed the untriaged New issue has not been triaged by the area owner label Dec 4, 2022
@davidfowl
Copy link
Member

davidfowl commented Dec 20, 2022

The current model is fundamentally incompatible with that @jackbond and if we do that then we should make a different type. The current configuration assumes you can access all of the keys in memory. What you're asking for is asynchronous access to individual keys. If we go that route then we should build a new API that doesn't overlap with the existing configuration provider.

@KieranDevvs
Copy link
Author

The alternative design looks better but did you also look at all of the consumers of the Load method to understand what else would need to be changed to async to make this work in practice? What about the generic hosting APIs?

Ok thanks for the feedback, I'm on xmas holiday at the moment, ill revisit this in the new year.

@davidfowl
Copy link
Member

davidfowl commented Dec 23, 2022

I don’t understand why you need asynchronous access to individual values and how did you get all of the keys in memory. IMO that’s another interface and a different API completely.

@davidfowl
Copy link
Member

Right, and I don't have a problem with having a new abstraction for stores like keyvault and azure app configuration. Seems very much like #36035.

If that's the goal of asynchrony, then I would opt for not polluting the existing API and designing a new one.

@davidfowl
Copy link
Member

For example, even reading a local json config file should be an async operation, it's doing async IO after all.

Loading everything up front (async or sync) is fine, and why the original API proposal (though I would change the existing interfaces and classes instead of making new ones) is easier to reason about with the existing configuration model.

Anything that requires asynchronous access to values should be a different abstraction. So maybe this IAsyncConfigurationProvider proposal should be about accessing values asynchronously. Instead of loading all keys up front somehow (Do most of those systems have a cheap way to query all keys? Do we even want that?).

I think these providers would be inappropriate for things like configuration binding, which want to know all of the keys (in the entire object hierarchy) before binding. This API would be used directly in the places that need access to individual values at the moment they are queried, without caching from the source.

@davidfowl
Copy link
Member

davidfowl commented Dec 24, 2022

Well I can't speak for all providers, but Azure.Data.AppConfiguration.ConfigurationClient has a method GetConfigurationSettingsAsync which allows you to read all the keys. It also loads all the values, but some of those values are merely links to the underlying KeyVault. We delay load those, and use .Result (which always bother me) in TryGet.

Right, to make general abstractions, we need to make sure the pattern holds broadly. I'm making an assumption that enumerating keys without values isn't common, but if we can find places where it is, then those assumptions can change.

@davidfowl
Copy link
Member

But at the same time, I do miss the old Microsoft days when the attitude was, "If you don't support our pattern, it's your problem" :)

That's ironic because the reason the azure configuration provider and keyvault provider is not ideal today is because the existing abstraction doesn't work well with the existing model😄

@davidfowl
Copy link
Member

davidfowl commented Dec 24, 2022

😄 True enough, although this feels more like an inhouse dispute, where IConfiguration simply hasn't received the modernization attention it deserves.

As one of the creators of this abstraction, that's not the case.

For example, how is the FileInfo class setting its Exists property without sneaking in an async operation?

FileProviders was never really meant to exist outside of the ASP.NET Core use case of being a readonly file system for web applications. I don't think it has any of the appropriate features for a general purpose file system abstraction (as much as people want it to be).

ServiceCollection should support BuildServiceProviderAsync

This doesn't make any sense to me and would make everything less efficient.

It's hard to do async all the way down when some libraries stop you dead in your tracks.

Agreed. It's a pain, but everything shouldn't be async or we can just throw performance out the window.

@davidfowl
Copy link
Member

To be clear, not saying it hasn't been maintained, just not "asyncified".

We're reluctant to do it without deeply understanding the usage. That's where you (customers) come in!

In regards to the FileInfo class, I'm not talking anything ASP.NET specific. System.IO.FileInfo.Exists could be potentially more than 50ms (is that still the guidance), I'd argue that it should have been Obsoleted and replaced with ExistsAsync a while back.

I don't agree. Unless we're talking about mapped network storage (cloud or not), file access is usually fast and cached at the layer. Deprecating Exists doesn't make sense IMO.

BuildServiceProvider is not something that you're running repeatedly in a tight loop (so less concerned about performance), but it is something that more than likely has async operations under the hood. For example, reading a connection string async, and passing that to a service like a db provider (which may actually like to initialize / warm up asynchronously.)

Build doesn't do anything async, resolution can though.

Unless the guidance has changed, async all the way is still the official line, and being forced to use .Result due to an external API doesn't change that, it just introduces the potential for a deadlock.

Right and everything shouldn't be async. The good news is though, making something async that was previously sync should force people to rethink all of the call sites and better understand why async is needed. The fact that it is viral is significant, and if it ends up being expensive (for example configuration binding), then we should re-think the interface and consumption.

As a side note, I'd like to thank you for all the work you do, and for taking time out to participate in discussions like this.

Thanks 🙏🏾

@KieranDevvs
Copy link
Author

KieranDevvs commented Jan 3, 2023

The current model is fundamentally incompatible with that @jackbond and if we do that then we should make a different type. The current configuration assumes you can access all of the keys in memory. What you're asking for is asynchronous access to individual keys. If we go that route then we should build a new API that doesn't overlap with the existing configuration provider.

I agree with this, if you're loading the values at the time of request i.e lazy loading, then that should be a new API, or even left to the user to implement as I cant think of many other scenarios where you would also want that behavior?

The alternative design looks better but did you also look at all of the consumers of the Load method to understand what else would need to be changed to async to make this work in practice? What about the generic hosting APIs?

The only consumer of Load as far as I can see, is the Build call at the time of building the IConfigurationRoot. As for your comment on generic hosting API's, those API's as well as anything else that would want to leverage async configuration sources, would need to implement the feature. The approach I'm proposing is such that it would be opt-in and wouldn't change any existing behavior.

@jjxtra
Copy link

jjxtra commented Mar 5, 2023

Agreed that an async interface and class should be made separately and would be used by a minority of people, but still necessary for those people.

@Rabadash8820
Copy link

Just adding a +1 here. I don't have strong feelings on whether there's a new interface or an addition to the existing IConfigurationBuilder interface, but some sort of async support is essential for scenarios with custom threading models. For example, I am trying to implement a configuration provider for Unity RemoteConfig. RemoteConfig exposes both a synchronous FetchConfigs and an asynchronous FetchConfigsAsync method, but I can't use either:

  • Waiting the Task returned by RemoteConfigService.FetchConfigsAsync fails as this would lead to a deadlock with the Unity main thread that calls IConfigurationBuilder.Build() (as the UnitySynchronizationContext essentially queues tasks on that thread, rather than using the thread pool)
  • FetchConfigs does not synchronously return settings, but invokes a FetchCompleted event to which calling code can subscribe, and this callback runs at an indeterminate time. My IConfigurationSource implementation could accept a callback and add that as a listener to FetchCompleted, but that breaks the encapsulation/contract of configuration sources. Consumers could also wait for a short time after after building configuration to ensure that FetchCompleted has been triggered, but the amount of time would be arbitrary--either too short or longer than necessary.

I've raised the latter concern on the Unity Forum, but an asynchronous version of IConfigurationBuilder.Load would also solve these issues, allowing calling code to await and play nicely with the UnitySynchronizationContext.

@klemmchr
Copy link

What's the status on this? Most configuration providers are asynchronous under the hood so this is desperately needed.

@KieranDevvs
Copy link
Author

KieranDevvs commented Nov 10, 2023

What's the status on this? Most configuration providers are asynchronous under the hood so this is desperately needed.

You can see the status. You can always contribute to help push this along. A proof of concept would be a good start, I just don't have the time to invest right now.

@robertmclaws
Copy link

@davidfowl Would be amazing if this could get into .NET 10...

@dazinator
Copy link

dazinator commented Jan 23, 2025

I [worked around this by calling an InitialiseAsync() on my provider to async prefetch the configuration values, ahead of adding it and calling ConfigurationBuilder.Build() which it can then return the perfetched configuration synchronously.

However this doesn't solve the issue of Reload and change tokens not being asynchronous.
When a configuration provider is signalled to reload that currently is also a synchronous operation. I had to get creative and implement an Async version of a change token to solve this issue.

@davidfowl I think this is more about the conflict of

  1. We are often told that network bound io calls should be asynchronous, many libraries provide asynchronous methods to make those calls.
  2. Configuration often has to be fetched from network initially.
  3. The HostBuilder and configuration builder experience doesn't provide any obvious location for performing this async configuration fetching, either during the initial build, or a config reload. I just create my provider and call InitialiseAsync() in program main before adding it to the builder, but this is "non standard".
  4. Change tokens signal config reloads. There is a seperate item somewhere about async change tokens, I think this might help.
  5. Is it OK to fetch config synchronously instead - probably yes, keeping hold of extra cycles for a smallish network call once at application startup or a few times during config reloads are runtime is most likely not a big issue. However its cognitively irksome to make network calls using synchronous api's and feels "wrong". Its also not a problem to do this asynchronously either in terms of "performance" that should not be the issue here.

@klemmchr
Copy link

To add another use case where we were facing issues:

We have a custom configuration provider in Blazor that makes an api call to get the current configuration for the web app. We are forced to do this since we're having different configurations per environment and do not want to rebuild our whole application when changing these or when rolling out our application.
When using Blazor WASM you cannot use blocking calls because it is running single threaded. Any attempt to use blocking calls (like GetAwaiter().GetResult() will yield a PlatformNotSupportedException (dotnet/aspnetcore#57429).

Our current approach matches the one from @dazinator: we create the provider beforehand, load the config in the main entry point (where we can use async) and then add it to the configuration builder. While this approach works fine, it has some drawbacks:

  1. It's clunky. We're bypassing all logic that the configuration provider has for loading configuration and we need to have quite some initialization code in our application just to support that use case.
  2. We cannot use config reloading at all. That's not the worse thing you could have but it would be great if we could reload our configuration from time to time to ensure that feature flags are up-to-date.

Besides this technical issues, this also feels weird for other reasons:

  1. We were told to use async everywhere. From simple file operations to network operations, everything is async these days. It feels counter intuitive to work around this just because the config provider does not support it.
  2. A lot of config providers actually need this. Even simple providers like file config providers should prefer async file APIs. Besides that, there are dozens of config providers that actually make network request that surely should not block a thread while doing this (https://github.com/Azure/AppConfiguration to name one)

Having support for an async provider should be optional, not mandatory. I don't think that introducing a breaking change in the IConfigurationProvider interface would be a good move, therefore I would go with a new interface IAsyncConfigurationProvider that could optionally inherit from IConfigurationProvider. Having a breaking change in the host builder would affect a lot of things already so I don't think breaking all existing configuration providers would be beneficial.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-Extensions-Configuration
Projects
None yet
Development

No branches or pull requests

9 participants