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

Dispose configuration in WebHost #9149

Merged

Conversation

poke
Copy link
Contributor

@poke poke commented Apr 6, 2019

This is a companion-PR to dotnet/extensions#1361 which addresses dotnet/extensions#938: When disposing the WebHost, the underlying configurations will also be disposed by the service provider.

Since the IConfiguration that is passed to the WebHost constructor is the host configuration, and does not access the configuration directly (the Startup may though), the WebHost has to resolve the app configuration from the service provider. This is done early within Initialize as soon as the service provider is available to avoid having to retrieve it later only for disposal.

The host configuration is also disposed by the WebHost transitively. While the host configuration only ever contains a single environment variables configuration provider (which is not disposable itself), disposing the configuration will also make sure to dispose the change registration.

The drawback for this is that using the WebHostBuilder.GetSetting() after the web host is disposed means that the operation is made on an already disposed configuration root. The way the configuration root is currently implemented, this should not actually prevent this from working though.

@poke
Copy link
Contributor Author

poke commented Apr 6, 2019

/cc @davidfowl

@analogrelay analogrelay added this to the 3.0.0-preview5 milestone Apr 8, 2019
@poke poke force-pushed the extensions-938-webhost-dispose-configuration branch from 66bb9e4 to b8317c9 Compare April 9, 2019 00:02
@poke
Copy link
Contributor Author

poke commented Apr 9, 2019

The disposal of the host configuration depends on the changes to ChainedConfigurationProvider in dotnet/extensions#1361, and is not covered by tests (because there’s no proper access to it).

@poke poke force-pushed the extensions-938-webhost-dispose-configuration branch from b8317c9 to 318a31b Compare April 9, 2019 23:07
@poke poke requested a review from dougbu as a code owner April 9, 2019 23:07
@poke
Copy link
Contributor Author

poke commented Apr 9, 2019

Since there wasn’t any PR from the bot over the course of the day that increased the version on the M.E.Configuration package, I just went totally rogue and edited the section that “should ONLY be updated by automation”… manually.

That way, I can at least compile the final version of this change and hopefully it does work on the CI end too.

If we need to wait for a proper update of the versions, then that’s of course fine with me too.

@Tratcher
Copy link
Member

Tratcher commented Apr 9, 2019

Yeah, we're going to need to wait for the rest of Extensions deps to get updated. Pulling just one isn't a good idea. @dougbu can you notify us here after the next update?

@poke
Copy link
Contributor Author

poke commented Apr 9, 2019

I don’t have the tools so that’s the best I can do right now 😁 And yeah, the CI really doesn’t like this apparently 😅

But at least I could push the latest code out, so you can look over it even if it’s not ready to merge yet.

@dougbu
Copy link
Member

dougbu commented Apr 10, 2019

@dougbu can you notify us here after the next update?

We should get new packages from EntityFrameworkCore and AspNetCore-Tooling every day. Those pull in the latest coherent Extensions packages. Coherency usually means Extensions build from day 1 is pulled into EF and Tooling on day 2 and those are pulled into this repo on day 3. Of course, the packages are all reving in parallel.

Which specific Extensions build are you looking for?

@Tratcher
Copy link
Member

@dougbu 3.0.0-preview5.19208.3 or later

@dougbu
Copy link
Member

dougbu commented Apr 10, 2019

🆗 3.0.0-preview5.19208.3 Extensions packages should be in AspNetCore tomorrow morning. AspNetCore-Tooling and EntityFrameworkCore have them now. And, both have had successful official builds since taking those dependencies.

@poke poke force-pushed the extensions-938-webhost-dispose-configuration branch from 318a31b to a4a3656 Compare April 10, 2019 21:10
@poke
Copy link
Contributor Author

poke commented Apr 10, 2019

The dependencies arrived and the PR is updated! Should be good to go now 😊

@Tratcher
Copy link
Member

[Ongoing build outage, I'll followup]

@poke
Copy link
Contributor Author

poke commented Apr 10, 2019

Yeah, I know, I’m already keeping an eye on #9263 and am ready to rebase to make the CI green.

Register the application configuration as a factory to make it dispose
automatically when the service provider gets disposed. This will dispose
the underlying configuration providers and change token registrations.

The host configuration will be disposed implicitly when the chained
configuration provider within the app configuration gets disposed.
@poke poke force-pushed the extensions-938-webhost-dispose-configuration branch from a4a3656 to 11e1462 Compare April 11, 2019 09:50
@poke
Copy link
Contributor Author

poke commented Apr 11, 2019

It’s green! 🎉 🎉

@Tratcher Tratcher merged commit bcad853 into dotnet:master Apr 11, 2019
@Tratcher
Copy link
Member

Thanks :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-hosting Includes Hosting
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants