-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Comments
Tagging subscribers to this area: @dotnet/area-extensions-configuration Issue DetailsBackground and motivationAs it currently stands, configuration sources have no way of being asynchronous (Highlighted in #36018). API Proposalnamespace 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 Usageinternal 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 DesignsIt 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();
} RisksNo response
|
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 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. |
Ok thanks for the feedback, I'm on xmas holiday at the moment, ill revisit this in the new year. |
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. |
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. |
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 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. |
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. |
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😄 |
As one of the creators of this abstraction, that's not the case.
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).
This doesn't make any sense to me and would make everything less efficient.
Agreed. It's a pain, but everything shouldn't be async or we can just throw performance out the window. |
We're reluctant to do it without deeply understanding the usage. That's where you (customers) come in!
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.
Build doesn't do anything async, resolution can though.
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.
Thanks 🙏🏾 |
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 only consumer of |
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. |
Just adding a +1 here. I don't have strong feelings on whether there's a new interface or an addition to the existing
I've raised the latter concern on the Unity Forum, but an asynchronous version of |
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. |
@davidfowl Would be amazing if this could get into .NET 10... |
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 @davidfowl I think this is more about the conflict of
|
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. 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:
Besides this technical issues, this also feels weird for other reasons:
Having support for an async provider should be optional, not mandatory. I don't think that introducing a breaking change in the |
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
API Usage
ConfigurationBuilder.Build()
would call Load() for allIConfigurationProvider
's, if async providers are registered, an exception is thrown.ConfigurationBuilder.BuildAsync()
would callLoad()
for allIConfigurationProvider
's andawait LoadAsync()
for allIAsyncConfigurationProvider
'sAlternative 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 thanConfigurationBuilder.Build()
.The text was updated successfully, but these errors were encountered: