[Host] Allow disabling reloadOnChange for Host's CreateDefaultBuilder #2940

kjerk · 2020-02-05T23:19:18Z

Add ASPNETCORE_BUILDER_CONFIG_RELOAD environment flag to allow disabling live reload in builder.
Add two tests to assert two different cases (live reload on, live reload off) work.

Addresses #2939

src/Hosting/Hosting/src/Host.cs

src/Hosting/Hosting/test/HostTests.cs

Tratcher · 2020-02-09T04:46:27Z

src/Hosting/Hosting/src/Host.cs

@@ -70,9 +70,11 @@ public static IHostBuilder CreateDefaultBuilder(string[] args)
            builder.ConfigureAppConfiguration((hostingContext, config) =>
            {
                var env = hostingContext.HostingEnvironment;
+
+                var reloadOnChange = hostingContext.Configuration.GetValue("HOSTBUILDER_CONFIG_RELOAD", true);


The host uses top level keys like contentRoot.
https://docs.microsoft.com/en-us/aspnet/core/fundamentals/host/generic-host?view=aspnetcore-3.1#contentrootpath
configReload should be adequate here.

Is there some prefix I can add to that to disambiguate? I was trying to follow the {ASPNETCORE/DOTNET}_FEATURE_DESCRIPTION nomenclature of other aspnetcore and dotnet flags sort of like DOTNET_USE_POLLING_FILE_WATCHER

This would mean that DOTNET_CONFIGRELOAD specified in an environment variable would trip the behavior added in this PR, and that to me sounds like it's entirely disabling config reloading, which is not quite the case, as one's own calls to AddJsonFile are left alone.

We can at least use the hierarchical config pattern.
Also, config keys are not case sensitive. Changed for readability.

Suggested change

var reloadOnChange = hostingContext.Configuration.GetValue("HOSTBUILDER_CONFIG_RELOAD", true);

var reloadFlagConfig = new Dictionary<string, string>() {{ "hostbuilder:configreload", "false" }};

Sounds good, this has been updated in my most recent commit de16b88

…d of runtime default.

Tratcher · 2020-02-14T17:20:02Z

src/Hosting/Hosting/src/Host.cs

@@ -70,9 +70,11 @@ public static IHostBuilder CreateDefaultBuilder(string[] args)
            builder.ConfigureAppConfiguration((hostingContext, config) =>
            {
                var env = hostingContext.HostingEnvironment;
+
+                var reloadOnChange = hostingContext.Configuration.GetValue("HOSTBUILDER_CONFIG_RELOAD", true);


We can at least use the hierarchical config pattern.
Also, config keys are not case sensitive. Changed for readability.

Suggested change

var reloadOnChange = hostingContext.Configuration.GetValue("HOSTBUILDER_CONFIG_RELOAD", true);

var reloadFlagConfig = new Dictionary<string, string>() {{ "hostbuilder:configreload", "false" }};

src/Hosting/Hosting/test/HostTests.cs

…o be longer and cancel using the reload token.

kjerk · 2020-03-04T00:07:26Z

Sorry it took a while to get back around to this one, and thanks for the feedback. I pushed de16b88 to resolve (I think) any dangling parts from the conversations.

Changelist

Update config key to hostbuilder:configreload
Changed both tests to async Task instead of being non async methods.
For the positive test case, increase wait time on test, listen for config.GetReloadToken() to fire and if it does, cancel any waiting and proceed.
Cancelling a task throws, so rather than write a full try/catch for the whole thing, wrap Task.Delay() in Task.WhenAny() which will ignore the throw and function as normal to keep it terse.

Tratcher

@shirhatti can you sanity check this for usability? CreateDefaultBuilder reads an entry "hostbuilder:configreload" from host config to enable/disable reload for appsettings.json.

src/Hosting/Hosting/src/Host.cs

analogrelay

Seems ok to me. I’ll leave it to @Tratcher to approve though.

src/Hosting/Hosting/test/HostTests.cs

ghost · 2020-03-19T16:36:19Z

Hello @Tratcher!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

Tratcher · 2020-03-19T16:42:15Z

Thanks @kjerk

kjerk · 2020-03-19T19:22:35Z

Thanks @kjerk

Thanks everybody.
Seems like a small change but this helps our prod deployments in Centos a lot.

…dotnet/extensions#2940) * Add environment var to allow disabling live reload in default builder * Switch to using hostingContext, rename flag to fit nomenclature. * Change flag to memory data source. Change content root default instead of runtime default. * Update config key to be hierarchical. Change await on positive case to be longer and cancel using the reload token. * Uncomment a development test case. * Apply suggestions from code review Co-authored-by: Chris Ross <Tratcher@Outlook.com> Commit migrated from dotnet/extensions@64140f9

davidfowl · 2020-07-16T07:14:26Z

The name of this configuration entry isn't ideal. Maybe it should be host and not hostbuilder?

am11 · 2021-03-08T01:42:54Z

src/Hosting/Hosting/src/Host.cs

@@ -70,9 +70,11 @@ public static IHostBuilder CreateDefaultBuilder(string[] args)
            builder.ConfigureAppConfiguration((hostingContext, config) =>
            {
                var env = hostingContext.HostingEnvironment;
+
+                var reloadOnChange = hostingContext.Configuration.GetValue("hostBuilder:reloadConfigOnChange", defaultValue: true);


Should this also be set to https://github.com/dotnet/runtime/blob/79ae74f5ca5c8a6fe3a48935e85bd7374959c570/src/libraries/Microsoft.Extensions.Configuration.FileExtensions/src/FileConfigurationSource.cs#L33?

Apparently this condition is true: https://github.com/dotnet/runtime/blob/79ae74f5ca5c8a6fe3a48935e85bd7374959c570/src/libraries/Microsoft.Extensions.Configuration.FileExtensions/src/FileConfigurationProvider.cs#L34 even with "hostBuilder": {"reloadConfigOnChange": false } in appsettings.json.

kernel implementation without inotify support throw the following exception on application startup and there seems to be no way to turn (this quite an unimportant) feature off:

Unhandled exception. System.IO.IOException: Function not implemented at System.IO.FileSystemWatcher.StartRaisingEvents() at System.IO.FileSystemWatcher.StartRaisingEventsIfNotDisposed() at System.IO.FileSystemWatcher.set_EnableRaisingEvents(Boolean value) at Microsoft.Extensions.FileProviders.Physical.PhysicalFilesWatcher.TryEnableFileSystemWatcher() at Microsoft.Extensions.FileProviders.Physical.PhysicalFilesWatcher.CreateFileChangeToken(String filter) at Microsoft.Extensions.FileProviders.PhysicalFileProvider.Watch(String filter) at Microsoft.Extensions.Configuration.FileConfigurationProvider.<.ctor>b__1_0() at Microsoft.Extensions.Primitives.ChangeToken.OnChange(Func`1 changeTokenProducer, Action changeTokenConsumer) at Microsoft.Extensions.Configuration.FileConfigurationProvider..ctor(FileConfigurationSource source) at Microsoft.Extensions.Configuration.Json.JsonConfigurationSource.Build(IConfigurationBuilder builder) at Microsoft.Extensions.Configuration.ConfigurationBuilder.Build() at Microsoft.Extensions.Hosting.HostBuilder.BuildAppConfiguration() at Microsoft.Extensions.Hosting.HostBuilder.Build() at mymvc1.Program.Main(String[] args)

got the global opt-out knob working with: dotnet/runtime@457ed11; DOTNET_USE_POLLING_FILE_WATCHER=1 in unikernel environment completely disables inotify. sadly, it wasn't backported to net5 so i switched to net6..

At least for HostBuilder, at the time this was being changed if you used CreateDefaultBuilder() you were getting live reloading whether you wanted it or not because it was hard coded to true

If Source.ReloadOnChange is populated by an env var and is available by the time ConfigureAppConfiguration() fires then that would work a bit better.

davidfowl · 2021-03-08T01:45:53Z

What did we merge here? Did we change the key to host vs hostBuilder?

kjerk · 2021-03-08T23:18:05Z

Looking back at this it looks like the config change was mentioned a few months after it was merged/closed (Mar 19).

I think iirc at the time when referencing the docs, while Host.blah was used as a static factory method, most things were referencing IHostBuilder, CreateHostBuilder, CreateDefaultBuilder, etc etc, and the only other ENV var override I could find referenced the implementation class, so I just defaulted to hostBuilder, though host is a bit nicer and agnostic sounding.

To make the change would require subsequent changes and maybe another PR?

Also because I myself had trouble finding the tiny link at the top of this PR, the original issue this is resolving is here: #2939 which gives some details on what problem this was addressing.

drdamour · 2022-05-24T15:49:10Z

was ASPNETCORE_BUILDER_CONFIG_RELOAD used, i can't find any reference to it through google, or was this solved some totally different way?

Tratcher · 2022-05-24T16:26:34Z

https://github.com/dotnet/runtime/blob/02ad981eab46339b6abef5b97b3db761d22b6455/src/libraries/Microsoft.Extensions.Hosting/src/HostingHostBuilderExtensions.cs#L237
It would be DOTNET_hostBuilder_reloadConfigOnChange=true

Add environment var to allow disabling live reload in default builder

1b2d0b6

kjerk requested review from analogrelay and Tratcher as code owners February 5, 2020 23:19

Tratcher added the area-hosting label Feb 5, 2020

kjerk mentioned this pull request Feb 6, 2020

[Host] Allow disabling reloadOnChange for Host's CreateDefaultBuilder #2939

Closed

Tratcher requested changes Feb 7, 2020

View reviewed changes

src/Hosting/Hosting/src/Host.cs Outdated Show resolved Hide resolved

Switch to using hostingContext, rename flag to fit nomenclature.

126381c

Tratcher requested changes Feb 9, 2020

View reviewed changes

src/Hosting/Hosting/test/HostTests.cs Outdated Show resolved Hide resolved

src/Hosting/Hosting/test/HostTests.cs Outdated Show resolved Hide resolved

src/Hosting/Hosting/test/HostTests.cs Outdated Show resolved Hide resolved

Tratcher reviewed Feb 9, 2020

View reviewed changes

Change flag to memory data source. Change content root default instea…

1a9b615

…d of runtime default.

Tratcher reviewed Feb 14, 2020

View reviewed changes

Tratcher self-assigned this Feb 14, 2020

kjerk added 2 commits March 3, 2020 15:59

Update config key to be hierarchical. Change await on positive case t…

de16b88

…o be longer and cancel using the reload token.

Uncomment a development test case.

51c3a03

Tratcher approved these changes Mar 4, 2020

View reviewed changes

src/Hosting/Hosting/src/Host.cs Outdated Show resolved Hide resolved

analogrelay reviewed Mar 19, 2020

View reviewed changes

Tratcher reviewed Mar 19, 2020

View reviewed changes

src/Hosting/Hosting/test/HostTests.cs Outdated Show resolved Hide resolved

Tratcher reviewed Mar 19, 2020

View reviewed changes

src/Hosting/Hosting/test/HostTests.cs Outdated Show resolved Hide resolved

Apply suggestions from code review

f0d48a3

Tratcher added the auto-merge label Mar 19, 2020

Tratcher added this to the 5.0.0-preview3 milestone Mar 19, 2020

ghost merged commit 64140f9 into dotnet:master Mar 19, 2020

kjerk deleted the HostBuilderDefaultReload branch March 19, 2020 19:21

am11 reviewed Mar 8, 2021

View reviewed changes

ghost locked as resolved and limited conversation to collaborators May 26, 2023

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Host] Allow disabling reloadOnChange for Host's CreateDefaultBuilder #2940

[Host] Allow disabling reloadOnChange for Host's CreateDefaultBuilder #2940

kjerk commented Feb 5, 2020

Tratcher Feb 9, 2020

kjerk Feb 11, 2020

Tratcher Feb 14, 2020

kjerk Mar 4, 2020

Tratcher Feb 14, 2020

kjerk commented Mar 4, 2020

Tratcher left a comment

analogrelay left a comment

ghost commented Mar 19, 2020

Tratcher commented Mar 19, 2020

kjerk commented Mar 19, 2020

davidfowl commented Jul 16, 2020

am11 Mar 8, 2021

am11 Mar 8, 2021

kjerk Mar 8, 2021

davidfowl commented Mar 8, 2021

kjerk commented Mar 8, 2021

drdamour commented May 24, 2022

Tratcher commented May 24, 2022

	var reloadOnChange = hostingContext.Configuration.GetValue("HOSTBUILDER_CONFIG_RELOAD", true);
	var reloadFlagConfig = new Dictionary<string, string>() {{ "hostbuilder:configreload", "false" }};

[Host] Allow disabling reloadOnChange for Host's CreateDefaultBuilder #2940

[Host] Allow disabling reloadOnChange for Host's CreateDefaultBuilder #2940

Conversation

kjerk commented Feb 5, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kjerk commented Mar 4, 2020

Tratcher left a comment

Choose a reason for hiding this comment

analogrelay left a comment

Choose a reason for hiding this comment

ghost commented Mar 19, 2020

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

Tratcher commented Mar 19, 2020

kjerk commented Mar 19, 2020

davidfowl commented Jul 16, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidfowl commented Mar 8, 2021

kjerk commented Mar 8, 2021

drdamour commented May 24, 2022

Tratcher commented May 24, 2022

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.