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]: IConfigurationRoot should derive from IDisposable #86456

Open
adamsitnik opened this issue May 18, 2023 · 4 comments
Open

[API Proposal]: IConfigurationRoot should derive from IDisposable #86456

adamsitnik opened this issue May 18, 2023 · 4 comments
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-Extensions-Configuration
Milestone

Comments

@adamsitnik
Copy link
Member

adamsitnik commented May 18, 2023

Background and motivation

When working on #86146 I've realized that ConfigurationBuilder.Build returns an instance of IConfigurationRoot, which does not derive from IDisposable, but all implementations of IConfigurationRoot implement IDisposable. So the users need to cast the result returned by ConfigurationBuilder.Build to IDisposable and call Dispose themselves to cleanup the resources properly.

IConfigurationRoot config = new ConfigurationBuilder().AddXmlFile("settings.xml", false, true).Build();

(config as IDisposable)?.Dispose();

API Proposal

namespace Microsoft.Extensions.Configuration;

- public interface IConfigurationRoot : IConfiguration
+ public interface IConfigurationRoot : IConfiguration, IDisposable
{
    /// <summary>
    /// Force the configuration values to be reloaded from the underlying <see cref="IConfigurationProvider"/>s.
    /// </summary>
    void Reload();

    /// <summary>
    /// The <see cref="IConfigurationProvider"/>s for this configuration.
    /// </summary>
    IEnumerable<IConfigurationProvider> Providers { get; }
}

API Usage

using IConfigurationRoot config = new ConfigurationBuilder().AddXmlFile("settings.xml", false, true).Build();

Alternative Designs

No response

Risks

Any custom implementations of IConfigurationRoot that don't implement IDisposable will have to implement it, moreover users of IConfigurationRoot will most likely get a compiler warning about not disposing the instance.

Without introducing the mentioned changes we risk common resource leaks like in #86146.

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

ghost commented May 18, 2023

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

When working on #86146 I've realized that ConfigurationBuilder.Build returns an instance of IConfigurationRoot, which does not derive from IDisposable, but all implementations of IConfigurationRoot implement IDisposable. So the users need to cast the result returned by ConfigurationBuilder.Build to IDisposable and call Dispose themselves to cleanup the resources properly.

IConfigurationRoot config = new ConfigurationBuilder().AddXmlFile("settings.xml", false, true).Build();

(config as IDisposable)?.Dispose();

API Proposal

namespace Microsoft.Extensions.Configuration;

- public interface IConfigurationRoot : IConfiguration
+ public interface IConfigurationRoot : IConfiguration, IDisposable
{
    /// <summary>
    /// Force the configuration values to be reloaded from the underlying <see cref="IConfigurationProvider"/>s.
    /// </summary>
    void Reload();

    /// <summary>
    /// The <see cref="IConfigurationProvider"/>s for this configuration.
    /// </summary>
    IEnumerable<IConfigurationProvider> Providers { get; }
}

API Usage

using IConfigurationRoot config = new ConfigurationBuilder().AddXmlFile("settings.xml", false, true).Build();

Alternative Designs

No response

Risks

Any custom implementations of IConfigurationRoot that don't implement IDisposable will have to implement it.

Without introducing the mentioned changes we risk common resource leaks like in #86146.

Author: adamsitnik
Assignees: -
Labels:

api-suggestion, untriaged, area-Extensions-Configuration

Milestone: -

@adamsitnik adamsitnik removed the untriaged New issue has not been triaged by the area owner label May 18, 2023
@tarekgh tarekgh added this to the Future milestone May 18, 2023
@ericstj
Copy link
Member

ericstj commented Apr 5, 2024

I'm not sure the IConfigurationRoot is solely responsible for owning the FileProvider. Instead I think we should try to look at what's the best practice for lifetime management (and instance management) of the FileProvider. IMO it should be higher in the stack here. Might make sense to ask the original architects what they had in mind @davidfowl @eerhardt @halter73

@adamsitnik
Copy link
Member Author

I'm not sure the IConfigurationRoot is solely responsible for owning the FileProvider.

My main goal of this proposal was to prevent similar resource leaks by letting the users know that ConfigurationBuilder.Build always returns something that implements IDisposable. I expect that very few users perform following IDisposable check:

IConfigurationRoot config = new ConfigurationBuilder().Build();

(config as IDisposable)?.Dispose(); // this is the problem

In most cases it's probably fine, as the config usually lives as long as the app does. But it's definitely not 100% of all the use cases (a good example are test projects which may create a lot of such instances).

@ericstj
Copy link
Member

ericstj commented Apr 26, 2024

My point was that I'm not convinced that IConfigurationRoot / IConfigurationProvider should be the one's who own the file provider. For them to solely own it - that's an awful lot of FileProviders we end up creating. So if they don't own it then some other component would own it and these would need to implement ref-counting.

I think a better solution is that we have a single FileProvider and try to plumb the lifetime of that into some part of the app model so that we don't need ref counting.

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

3 participants