Skip to content

Commit 29397ab

Browse files
mathewcmathewc
committed
Fixing IConfiguration environment variable caching
1 parent 2c93c87 commit 29397ab

22 files changed

+148
-201
lines changed

‎src/WebJobs.Script.WebHost/Management/InstanceManager.cs‎

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -17,14 +17,12 @@ public class InstanceManager : IInstanceManager
1717
private static HostAssignmentContext _assignmentContext;
1818
private static object _assignmentLock = new object();
1919

20-
private readonly ScriptHostManager _scriptHostManager;
2120
private readonly WebHostSettings _webHostSettings;
2221
private readonly ILogger _logger;
2322
private readonly HttpClient _client;
2423

25-
public InstanceManager(WebScriptHostManager scriptHostManager, WebHostSettings webHostSettings, ILoggerFactory loggerFactory, HttpClient client)
24+
public InstanceManager(WebHostSettings webHostSettings, ILoggerFactory loggerFactory, HttpClient client)
2625
{
27-
_scriptHostManager = scriptHostManager;
2826
_webHostSettings = webHostSettings;
2927
_logger = loggerFactory.CreateLogger(nameof(InstanceManager));
3028
_client = client;
@@ -77,8 +75,7 @@ private async Task Specialize(HostAssignmentContext assignmentContext)
7775
assignmentContext.ApplyAppSettings();
7876
}
7977

80-
// Restart runtime.
81-
_scriptHostManager.RestartHost();
78+
// TODO: specialize
8279
}
8380
catch (Exception ex)
8481
{

‎src/WebJobs.Script.WebHost/Models/HostAssignmentContext.cs‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -53,12 +53,10 @@ public bool Equals(HostAssignmentContext other)
5353

5454
public void ApplyAppSettings()
5555
{
56-
// apply app settings
5756
foreach (var pair in Environment)
5857
{
5958
System.Environment.SetEnvironmentVariable(pair.Key, pair.Value);
6059
}
61-
ScriptSettingsManager.Instance.Reset();
6260
}
6361
}
6462
}

‎src/WebJobs.Script.WebHost/Program.cs‎

Lines changed: 24 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,11 @@
11
// Copyright (c) .NET Foundation. All rights reserved.
22
// Licensed under the MIT License. See License.txt in the project root for license information.
33

4-
using System;
4+
using System.Linq;
55
using System.Threading;
66
using Microsoft.AspNetCore.Hosting;
7+
using Microsoft.Extensions.Configuration;
8+
using Microsoft.Extensions.Configuration.EnvironmentVariables;
79

810
namespace Microsoft.Azure.WebJobs.Script.WebHost
911
{
@@ -18,10 +20,27 @@ public static void Main(string[] args)
1820
.Wait();
1921
}
2022

21-
public static IWebHost BuildWebHost(string[] args) =>
22-
Microsoft.AspNetCore.WebHost.CreateDefaultBuilder(args)
23-
.UseStartup<Startup>()
24-
.Build();
23+
public static IWebHost BuildWebHost(string[] args)
24+
{
25+
return CreateWebHostBuilder(args).Build();
26+
}
27+
28+
public static IWebHostBuilder CreateWebHostBuilder(string[] args = null)
29+
{
30+
return Microsoft.AspNetCore.WebHost.CreateDefaultBuilder(args)
31+
.ConfigureAppConfiguration((builderContext, config) =>
32+
{
33+
// replace the default environment source with our own
34+
IConfigurationSource envVarsSource = config.Sources.OfType<EnvironmentVariablesConfigurationSource>().FirstOrDefault();
35+
if (envVarsSource != null)
36+
{
37+
config.Sources.Remove(envVarsSource);
38+
}
39+
envVarsSource = new ScriptEnvironmentVariablesConfigurationSource();
40+
config.Sources.Add(envVarsSource);
41+
})
42+
.UseStartup<Startup>();
43+
}
2544

2645
internal static void InitiateShutdown()
2746
{

‎src/WebJobs.Script.WebHost/WebHostResolver.cs‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -112,7 +112,6 @@ internal void EnsureInitialized(WebHostSettings settings)
112112
if (_activeHostManager == null &&
113113
(_standbyHostManager == null || _settingsManager.ContainerReady))
114114
{
115-
_settingsManager.Reset();
116115
_specializationTimer?.Dispose();
117116
_specializationTimer = null;
118117

‎src/WebJobs.Script.WebHost/WebJobsServiceCollectionExtensions.cs‎

Lines changed: 10 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -72,12 +72,11 @@ public static IServiceProvider AddWebJobsScriptHost(this IServiceCollection serv
7272

7373
// ScriptSettingsManager should be replaced. We're setting this here as a temporary step until
7474
// broader configuaration changes are made:
75-
var settingsManager = ScriptSettingsManager.Instance;
76-
settingsManager.SetConfigurationFactory(() => configuration);
77-
builder.RegisterInstance(settingsManager);
75+
builder.RegisterType<ScriptSettingsManager>().SingleInstance();
7876

7977
builder.Register<IEventGenerator>(c =>
8078
{
79+
var settingsManager = c.Resolve<ScriptSettingsManager>();
8180
if (settingsManager.IsLinuxContainerEnvironment)
8281
{
8382
return new LinuxContainerEventGenerator();
@@ -92,13 +91,15 @@ public static IServiceProvider AddWebJobsScriptHost(this IServiceCollection serv
9291
builder.RegisterType<ScriptEventManager>().As<IScriptEventManager>().SingleInstance();
9392
builder.Register(c => WebHostSettings.CreateDefault(c.Resolve<ScriptSettingsManager>()));
9493

95-
// Pass a specially-constructed LoggerFactory to the WebHostResolver. This LoggerFactory is only used
96-
// when there is no host available.
94+
// Pass a specially-constructed LoggerFactory to the WebHostResolver, if there isn't already a service instance
95+
// registered. This LoggerFactory is only used when there is no host available.
9796
// Only use this LoggerFactory for this constructor; use the registered ILoggerFactory below everywhere else.
97+
// TODO: This default logger factory is a TEMP workaround. Fix this.
98+
var defaultLoggerFactory = services.Where(p => p.ServiceType == typeof(ILoggerFactory) && p.ImplementationInstance != null).Select(p => p.ImplementationInstance).FirstOrDefault();
9899
builder.RegisterType<WebHostResolver>().SingleInstance()
99-
.WithParameter(new ResolvedParameter(
100-
(pi, ctx) => pi.ParameterType == typeof(ILoggerFactory),
101-
(pi, ctx) => CreateLoggerFactory(string.Empty, ctx.Resolve<ScriptSettingsManager>(), ctx.Resolve<IEventGenerator>(), ctx.Resolve<WebHostSettings>())));
100+
.WithParameter(new ResolvedParameter(
101+
(pi, ctx) => pi.ParameterType == typeof(ILoggerFactory),
102+
(pi, ctx) => defaultLoggerFactory ?? CreateLoggerFactory(string.Empty, ctx.Resolve<ScriptSettingsManager>(), ctx.Resolve<IEventGenerator>(), ctx.Resolve<WebHostSettings>())));
102103

103104
// Register the LoggerProviderFactory, which defines the ILoggerProviders for the host.
104105
builder.RegisterType<WebHostLoggerProviderFactory>().As<ILoggerProviderFactory>().SingleInstance();
@@ -124,6 +125,7 @@ public static IServiceProvider AddWebJobsScriptHost(this IServiceCollection serv
124125
// override the registrations above
125126
builder.Populate(services);
126127

128+
// we want all ILoggerFactory resolution to go through WebHostResolver
127129
builder.Register(ct => ct.Resolve<WebHostResolver>().GetLoggerFactory(ct.Resolve<WebHostSettings>())).As<ILoggerFactory>().ExternallyOwned();
128130

129131
var applicationContainer = builder.Build();

‎src/WebJobs.Script/Config/ScriptEnvironmentVariablesConfigurationSource.cs‎

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,40 @@
1+
// Copyright (c) .NET Foundation. All rights reserved.
2+
// Licensed under the MIT License. See License.txt in the project root for license information.
3+
4+
using System;
5+
using Microsoft.Extensions.Configuration;
6+
using Microsoft.Extensions.Configuration.EnvironmentVariables;
7+
8+
namespace Microsoft.Azure.WebJobs.Script
9+
{
10+
/// <summary>
11+
/// This source exists to replace the default <see cref="EnvironmentVariablesConfigurationSource"/> to allow
12+
/// us to override caching behavior.
13+
/// </summary>
14+
public class ScriptEnvironmentVariablesConfigurationSource : IConfigurationSource
15+
{
16+
public IConfigurationProvider Build(IConfigurationBuilder builder)
17+
{
18+
return new WebHostEnvironmentVariablesProvider();
19+
}
20+
21+
private class WebHostEnvironmentVariablesProvider : EnvironmentVariablesConfigurationProvider
22+
{
23+
public WebHostEnvironmentVariablesProvider() : base()
24+
{
25+
}
26+
27+
public override bool TryGet(string key, out string value)
28+
{
29+
value = Environment.GetEnvironmentVariable(key);
30+
31+
return value != null;
32+
}
33+
34+
public override void Set(string key, string value)
35+
{
36+
Environment.SetEnvironmentVariable(key, value);
37+
}
38+
}
39+
}
40+
}

‎src/WebJobs.Script/Config/ScriptSettingsManager.cs‎

Lines changed: 17 additions & 75 deletions
Original file line numberDiff line numberDiff line change
@@ -2,27 +2,23 @@
22
// Licensed under the MIT License. See License.txt in the project root for license information.
33

44
using System;
5-
using System.Collections.Concurrent;
65
using Microsoft.Extensions.Configuration;
76

87
namespace Microsoft.Azure.WebJobs.Script.Config
98
{
109
public class ScriptSettingsManager
1110
{
1211
private static ScriptSettingsManager _instance = new ScriptSettingsManager();
13-
private readonly ConcurrentDictionary<string, string> _settingsCache = new ConcurrentDictionary<string, string>();
14-
private Func<IConfiguration> _configurationFactory = BuildConfiguration;
15-
private Lazy<IConfiguration> _configuration = new Lazy<IConfiguration>(BuildConfiguration);
1612

17-
// for testing
18-
public ScriptSettingsManager()
13+
public ScriptSettingsManager(IConfiguration config = null)
1914
{
15+
Configuration = config ?? BuildDefaultConfiguration();
2016
}
2117

2218
/// <summary>
2319
/// Gets the underlying configuration object used by this instance of the <see cref="ScriptSettingsManager"/>.
2420
/// </summary>
25-
internal IConfiguration Configuration => _configuration.Value;
21+
internal IConfiguration Configuration { get; }
2622

2723
public static ScriptSettingsManager Instance
2824
{
@@ -62,19 +58,15 @@ public virtual string AzureWebsiteDefaultSubdomain
6258
{
6359
get
6460
{
65-
return _settingsCache.GetOrAdd(nameof(AzureWebsiteDefaultSubdomain), k =>
66-
{
67-
string siteHostName = GetSetting(EnvironmentSettingNames.AzureWebsiteHostName);
68-
69-
int? periodIndex = siteHostName?.IndexOf('.');
61+
string siteHostName = GetSetting(EnvironmentSettingNames.AzureWebsiteHostName);
7062

71-
if (periodIndex != null && periodIndex > 0)
72-
{
73-
return siteHostName.Substring(0, periodIndex.Value);
74-
}
63+
int? periodIndex = siteHostName?.IndexOf('.');
64+
if (periodIndex != null && periodIndex > 0)
65+
{
66+
return siteHostName.Substring(0, periodIndex.Value);
67+
}
7568

76-
return null;
77-
});
69+
return null;
7870
}
7971
}
8072

@@ -111,42 +103,8 @@ public virtual string InstanceId
111103

112104
public virtual string ApplicationInsightsInstrumentationKey
113105
{
114-
get => GetSettingFromCache(EnvironmentSettingNames.AppInsightsInstrumentationKey);
115-
set => UpdateSettingInCache(EnvironmentSettingNames.AppInsightsInstrumentationKey, value);
116-
}
117-
118-
public void SetConfigurationFactory(Func<IConfiguration> configurationRootFactory)
119-
{
120-
_configurationFactory = configurationRootFactory;
121-
Reset();
122-
}
123-
124-
private string GetSettingFromCache(string settingKey)
125-
{
126-
if (string.IsNullOrEmpty(settingKey))
127-
{
128-
throw new ArgumentNullException(nameof(settingKey));
129-
}
130-
131-
return _settingsCache.GetOrAdd(settingKey, (key) => GetSetting(key));
132-
}
133-
134-
private void UpdateSettingInCache(string settingKey, string settingValue)
135-
{
136-
if (string.IsNullOrEmpty(settingKey))
137-
{
138-
throw new ArgumentNullException(nameof(settingKey));
139-
}
140-
141-
_settingsCache.AddOrUpdate(settingKey, settingValue, (a, b) => settingValue);
142-
}
143-
144-
public virtual void Reset()
145-
{
146-
Lazy<IConfiguration> current = _configuration;
147-
_configuration = new Lazy<IConfiguration>(() => InitializeConfiguration(current));
148-
149-
_settingsCache.Clear();
106+
get => GetSetting(EnvironmentSettingNames.AppInsightsInstrumentationKey);
107+
set => SetSetting(EnvironmentSettingNames.AppInsightsInstrumentationKey, value);
150108
}
151109

152110
public virtual string GetSetting(string settingKey)
@@ -164,35 +122,19 @@ public virtual void SetSetting(string settingKey, string settingValue)
164122
if (!string.IsNullOrEmpty(settingKey))
165123
{
166124
Environment.SetEnvironmentVariable(settingKey, settingValue);
167-
Reset();
168125
}
169126
}
170127

171-
private IConfiguration InitializeConfiguration(Lazy<IConfiguration> currentConfiguration)
128+
public static IConfiguration BuildDefaultConfiguration()
172129
{
173-
var configuration = _configurationFactory();
174-
175-
// If the factory returned a cached instance
176-
// that is the same as the instance we previously had,
177-
// reload the configuration on that instance.
178-
if (configuration is IConfigurationRoot root &&
179-
currentConfiguration != null &&
180-
currentConfiguration.IsValueCreated &&
181-
object.ReferenceEquals(currentConfiguration.Value, root))
182-
{
183-
root.Reload();
184-
}
185-
186-
return configuration;
130+
return CreateDefaultConfigurationBuilder().Build();
187131
}
188132

189-
private static IConfigurationRoot BuildConfiguration()
133+
internal static IConfigurationBuilder CreateDefaultConfigurationBuilder()
190134
{
191-
var configurationBuilder = new ConfigurationBuilder()
135+
return new ConfigurationBuilder()
192136
.AddJsonFile("appsettings.json", optional: true)
193-
.AddEnvironmentVariables();
194-
195-
return configurationBuilder.Build();
137+
.Add(new ScriptEnvironmentVariablesConfigurationSource());
196138
}
197139
}
198140
}

‎test/WebJobs.Script.Tests.Integration/ApplicationInsights/ApplicationInsightsTestFixture.cs‎

Lines changed: 10 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,6 @@
55
using System.Collections.Generic;
66
using System.IO;
77
using System.Net.Http;
8-
using Microsoft.AspNetCore.Hosting;
98
using Microsoft.AspNetCore.TestHost;
109
using Microsoft.Azure.WebJobs.Script.Config;
1110
using Microsoft.Azure.WebJobs.Script.WebHost;
@@ -30,28 +29,22 @@ public ApplicationInsightsTestFixture(string scriptRoot, string testId)
3029
SecretsPath = Environment.CurrentDirectory // not used
3130
};
3231

33-
_testServer = new TestServer(
34-
AspNetCore.WebHost.CreateDefaultBuilder()
35-
.UseStartup<Startup>()
36-
.ConfigureServices(services =>
32+
var hostBuilder = Program.CreateWebHostBuilder()
33+
.ConfigureAppConfiguration((builderContext, config) =>
3734
{
38-
var settingsManager = new ScriptSettingsManager();
39-
settingsManager.SetConfigurationFactory(() =>
35+
config.AddInMemoryCollection(new Dictionary<string, string>
4036
{
41-
return new ConfigurationBuilder()
42-
.AddInMemoryCollection(new Dictionary<string, string>
43-
{
44-
[EnvironmentSettingNames.AppInsightsInstrumentationKey] = TestChannelLoggerProviderFactory.ApplicationInsightsKey
45-
})
46-
.AddEnvironmentVariables()
47-
.Build();
37+
[EnvironmentSettingNames.AppInsightsInstrumentationKey] = TestChannelLoggerProviderFactory.ApplicationInsightsKey
4838
});
49-
50-
services.Replace(new ServiceDescriptor(typeof(ScriptSettingsManager), settingsManager));
39+
})
40+
.ConfigureServices(services =>
41+
{
5142
services.Replace(new ServiceDescriptor(typeof(WebHostSettings), HostSettings));
5243
services.Replace(new ServiceDescriptor(typeof(ILoggerProviderFactory), new TestChannelLoggerProviderFactory(Channel)));
5344
services.Replace(new ServiceDescriptor(typeof(ISecretManager), new TestSecretManager()));
54-
}));
45+
});
46+
47+
_testServer = new TestServer(hostBuilder);
5548

5649
var scriptConfig = _testServer.Host.Services.GetService<WebHostResolver>().GetScriptHostConfiguration(HostSettings);
5750

@@ -90,7 +83,6 @@ public void Dispose()
9083
{
9184
_testServer?.Dispose();
9285
HttpClient?.Dispose();
93-
ScriptSettingsManager.Instance.Reset();
9486
}
9587
}
9688
}

0 commit comments

Comments
 (0)