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

ConfigurationProviders leak memory #861

Closed
davidfowl opened this issue Dec 21, 2018 · 8 comments
Closed

ConfigurationProviders leak memory #861

davidfowl opened this issue Dec 21, 2018 · 8 comments
Assignees

Comments

@davidfowl
Copy link
Member

using System;
using System.Threading.Tasks;
using Microsoft.Extensions.Configuration;

namespace WebApplication45
{
    public class Program
    {
        public static async Task Main(string[] args)
        {
            Console.WriteLine("STARTING");
            Console.ReadLine();
            CreateConfig();
            Console.WriteLine("DONE");
            Console.ReadLine();
        }

        private static void CreateConfig()
        {
            for (int i = 0; i < 1000; i++)
            {
                var config = new ConfigurationBuilder()
                               .SetBasePath(Environment.CurrentDirectory)
                               .AddJsonFile("appsettings.json", optional: false, reloadOnChange: true)
                               .Build();
            }
        }
    }
}

This roots tons of objects because of #786 they can't be cleaned up.

image

@poke
Copy link

poke commented Jan 7, 2019

Just wondering if this would be the way we would solve this:

  • Make ConfigurationRoot disposable
  • Dispose all disposable providers when disposing the ConfigurationRoot
  • In WebHost, dispose of the ConfigurationRoot when the web host gets disposed

If that’s correct, I would be interested to pick this up.

Since the configuration is also used in the WebHostBuilder I assume that we would need to add a way to dispose of the configuration root there as well (making the builder disposable)?

@davidfowl
Copy link
Member Author

Make ConfigurationRoot disposable

Yes.

Dispose all disposable providers when disposing the ConfigurationRoot

Yes

In WebHost, dispose of the ConfigurationRoot when the web host gets disposed

This should happen for free since the ConfigurationRoot is in the container.

@poke
Copy link

poke commented Jan 8, 2019

@davidfowl

This should happen for free since the ConfigurationRoot is in the container.

But doesn’t aspnet/DependencyInjection#462 prevent the configuration root from getting disposed since it is being registered as an instance?

@davidfowl
Copy link
Member Author

Ah, yes you're right! We need to do the same for the hosting environment as well.

@poke
Copy link

poke commented Jan 8, 2019

You mean make HostingEnvironment disposable and dispose the two file providers? Or maybe dispose of those manually (maybe through an extension method) since the hosting environment itself is just a POCO while an extension method does the actual work?

Also, since we agree to dispose the configuration root manually now, what is your opinion on disposing the original configuration root inside the web host builder? Looking at how it is used (and how the web host builder just allows to be built once), I guess we could also dispose the config after the web host is being built and forward _config to the _context.Configuration instead (which is the host’s config)?

@HaoK HaoK self-assigned this Jan 9, 2019
davidfowl pushed a commit that referenced this issue Jan 11, 2019
- Dispose disposable configuration providers and their change token
  registrations when disposing the configuration root.
- Dispose the change token registration when disposing a FileConfigurationProvider.
@davidfowl davidfowl added this to the 3.0.0-preview2 milestone Jan 11, 2019
@davidfowl davidfowl added the bug label Jan 11, 2019
@davidfowl
Copy link
Member Author

Fixed

@poke
Copy link

poke commented Jan 11, 2019

@davidfowl We still need the work on the AspNetCore side though. I'm working on that (currently struggling with getting the repo to build).

@davidfowl
Copy link
Member Author

@poke We actually need it in 2 places. It needs to go into the generic host (that's the default in 3.0) and in the WebHost for compat.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants