From 6909ef49321fd45fd0d7f5155859ff044c23a280 Mon Sep 17 00:00:00 2001 From: martincostello Date: Sat, 2 Mar 2019 12:20:22 +0000 Subject: [PATCH 1/4] Atomically swap config data Swap the configuration providers' data atomically, rather than directly changing the property, so that any enumeration of the dictionary running during the reload operation does not throw an InvalidOperationException due to the collectino being modified. Relates to #1189. --- .../src/EnvironmentVariablesConfigurationProvider.cs | 6 ++++-- .../src/KeyPerFileConfigurationProvider.cs | 7 +++++-- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/src/Configuration/Config.EnvironmentVariables/src/EnvironmentVariablesConfigurationProvider.cs b/src/Configuration/Config.EnvironmentVariables/src/EnvironmentVariablesConfigurationProvider.cs index 3089093ff16..108c639486a 100644 --- a/src/Configuration/Config.EnvironmentVariables/src/EnvironmentVariablesConfigurationProvider.cs +++ b/src/Configuration/Config.EnvironmentVariables/src/EnvironmentVariablesConfigurationProvider.cs @@ -48,7 +48,7 @@ public override void Load() internal void Load(IDictionary envVariables) { - Data = new Dictionary(StringComparer.OrdinalIgnoreCase); + var data = new Dictionary(StringComparer.OrdinalIgnoreCase); var filteredEnvVariables = envVariables .Cast() @@ -58,8 +58,10 @@ internal void Load(IDictionary envVariables) foreach (var envVariable in filteredEnvVariables) { var key = ((string)envVariable.Key).Substring(_prefix.Length); - Data[key] = (string)envVariable.Value; + data[key] = (string)envVariable.Value; } + + Data = data; } private static string NormalizeKey(string key) diff --git a/src/Configuration/Config.KeyPerFile/src/KeyPerFileConfigurationProvider.cs b/src/Configuration/Config.KeyPerFile/src/KeyPerFileConfigurationProvider.cs index 13541110e63..2e33b9dfcd1 100644 --- a/src/Configuration/Config.KeyPerFile/src/KeyPerFileConfigurationProvider.cs +++ b/src/Configuration/Config.KeyPerFile/src/KeyPerFileConfigurationProvider.cs @@ -31,12 +31,13 @@ private static string TrimNewLine(string value) /// public override void Load() { - Data = new Dictionary(StringComparer.OrdinalIgnoreCase); + var data = new Dictionary(StringComparer.OrdinalIgnoreCase); if (Source.FileProvider == null) { if (Source.Optional) { + Data = data; return; } @@ -61,10 +62,12 @@ public override void Load() { if (Source.IgnoreCondition == null || !Source.IgnoreCondition(file.Name)) { - Data.Add(NormalizeKey(file.Name), TrimNewLine(streamReader.ReadToEnd())); + data.Add(NormalizeKey(file.Name), TrimNewLine(streamReader.ReadToEnd())); } } } + + Data = data; } private string GetDirectoryName() From 2251093fac99799af6ee08d4ffeb251269410c00 Mon Sep 17 00:00:00 2001 From: martincostello Date: Sat, 2 Mar 2019 14:27:19 +0000 Subject: [PATCH 2/4] Add tests for binding during reload Add tests for binding behaviour during reload. --- .../test/EnvironmentVariablesTest.cs | 47 +++++++++++++++++++ .../Config.KeyPerFile/test/KeyPerFileTests.cs | 43 +++++++++++++++++ 2 files changed, 90 insertions(+) diff --git a/src/Configuration/Config.EnvironmentVariables/test/EnvironmentVariablesTest.cs b/src/Configuration/Config.EnvironmentVariables/test/EnvironmentVariablesTest.cs index ca282e35479..72504a552ad 100644 --- a/src/Configuration/Config.EnvironmentVariables/test/EnvironmentVariablesTest.cs +++ b/src/Configuration/Config.EnvironmentVariables/test/EnvironmentVariablesTest.cs @@ -1,7 +1,11 @@ // Copyright (c) .NET Foundation. All rights reserved. // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. +using System; using System.Collections; +using System.Collections.Generic; +using System.Threading; +using System.Threading.Tasks; using Microsoft.Extensions.Configuration.Test; using Xunit; @@ -148,5 +152,48 @@ public void ReplaceDoubleUnderscoreInEnvironmentVariables() Assert.Equal("connection", envConfigSrc.Get("data:ConnectionString")); Assert.Equal("System.Data.SqlClient", envConfigSrc.Get("ConnectionStrings:_db1_ProviderName")); } + + [Fact] + public void BindingDoesNotThrowIfReloadedDuringBinding() + { + var dic = new Dictionary + { + {"Number", "-2"}, + {"Text", "Foo"} + }; + var configurationBuilder = new ConfigurationBuilder(); + configurationBuilder.AddInMemoryCollection(dic); + configurationBuilder.AddEnvironmentVariables(); + var config = configurationBuilder.Build(); + + MyOptions options = null; + + using (var cts = new CancellationTokenSource(TimeSpan.FromMilliseconds(250))) + { + void ReloadLoop() + { + while (!cts.IsCancellationRequested) + { + config.Reload(); + } + } + + _ = Task.Run(ReloadLoop); + + while (!cts.IsCancellationRequested) + { + options = config.Get(); + } + } + + Assert.Equal(-2, options.Number); + Assert.Equal("Foo", options.Text); + } + + private sealed class MyOptions + { + public int Number { get; set; } + public string Text { get; set; } + } } } diff --git a/src/Configuration/Config.KeyPerFile/test/KeyPerFileTests.cs b/src/Configuration/Config.KeyPerFile/test/KeyPerFileTests.cs index 4de528c0116..838e62222d6 100644 --- a/src/Configuration/Config.KeyPerFile/test/KeyPerFileTests.cs +++ b/src/Configuration/Config.KeyPerFile/test/KeyPerFileTests.cs @@ -6,6 +6,8 @@ using System.Collections.Generic; using System.IO; using System.Text; +using System.Threading; +using System.Threading.Tasks; using Microsoft.Extensions.FileProviders; using Microsoft.Extensions.Primitives; using Xunit; @@ -179,6 +181,47 @@ public void CanUnIgnoreDefaultFiles() Assert.Equal("SecretValue1", config["ignore.Secret1"]); Assert.Equal("SecretValue2", config["Secret2"]); } + + [Fact] + public void BindingDoesNotThrowIfReloadedDuringBinding() + { + var testFileProvider = new TestFileProvider( + new TestFile("Number", "-2"), + new TestFile("Text", "Foo")); + + var config = new ConfigurationBuilder() + .AddKeyPerFile(o => o.FileProvider = testFileProvider) + .Build(); + + MyOptions options = null; + + using (var cts = new CancellationTokenSource(TimeSpan.FromMilliseconds(250))) + { + void ReloadLoop() + { + while (!cts.IsCancellationRequested) + { + config.Reload(); + } + } + + _ = Task.Run(ReloadLoop); + + while (!cts.IsCancellationRequested) + { + options = config.Get(); + } + } + + Assert.Equal(-2, options.Number); + Assert.Equal("Foo", options.Text); + } + + private sealed class MyOptions + { + public int Number { get; set; } + public string Text { get; set; } + } } class TestFileProvider : IFileProvider From b7845ac17b710638f13dc95cc0e8e846174a6cce Mon Sep 17 00:00:00 2001 From: martincostello Date: Sat, 2 Mar 2019 14:50:32 +0000 Subject: [PATCH 3/4] Add functional test for binding during reload Add a functional test for binding during reload of multiple configuration sources. --- .../ConfigurationTests.cs | 61 ++++++++++++++++++- ...sions.Configuration.FunctionalTests.csproj | 3 + 2 files changed, 62 insertions(+), 2 deletions(-) diff --git a/src/Configuration/test/Config.FunctionalTests/ConfigurationTests.cs b/src/Configuration/test/Config.FunctionalTests/ConfigurationTests.cs index 27c694c43be..f4d7f0c5df6 100644 --- a/src/Configuration/test/Config.FunctionalTests/ConfigurationTests.cs +++ b/src/Configuration/test/Config.FunctionalTests/ConfigurationTests.cs @@ -5,13 +5,13 @@ using System.Collections.Generic; using System.IO; using System.Linq; +using System.Threading; using System.Threading.Tasks; -using Microsoft.AspNetCore.Testing; using Microsoft.AspNetCore.Testing.xunit; using Microsoft.Extensions.Configuration.Ini; using Microsoft.Extensions.Configuration.Json; -using Microsoft.Extensions.Configuration.Xml; using Microsoft.Extensions.Configuration.UserSecrets; +using Microsoft.Extensions.Configuration.Xml; using Microsoft.Extensions.FileProviders; using Microsoft.Extensions.Primitives; using Xunit; @@ -944,6 +944,50 @@ public async Task TouchingFileWillReloadForUserSecrets() Assert.True(token.HasChanged); } + [ConditionalFact] + [OSSkipCondition(OperatingSystems.Linux, SkipReason = "File watching is flaky on non windows.")] + [OSSkipCondition(OperatingSystems.MacOSX, SkipReason = "File watching is flaky on non windows.")] + public void BindingDoesNotThrowIfReloadedDuringBinding() + { + WriteTestFiles(); + + var configurationBuilder = CreateBuilder(); + configurationBuilder.Add(new TestIniSourceProvider(_iniFile)); + configurationBuilder.Add(new TestJsonSourceProvider(_jsonFile)); + configurationBuilder.Add(new TestXmlSourceProvider(_xmlFile)); + configurationBuilder.AddEnvironmentVariables(); + configurationBuilder.AddCommandLine(new[] { "--CmdKey1=CmdValue1" }); + configurationBuilder.AddInMemoryCollection(_memConfigContent); + + var config = configurationBuilder.Build(); + + using (var cts = new CancellationTokenSource(TimeSpan.FromMilliseconds(250))) + { + void ReloadLoop() + { + while (!cts.IsCancellationRequested) + { + config.Reload(); + } + } + + _ = Task.Run(ReloadLoop); + + MyOptions options = null; + + while (!cts.IsCancellationRequested) + { + options = config.Get(); + } + + Assert.Equal("CmdValue1", options.CmdKey1); + Assert.Equal("IniValue1", options.IniKey1); + Assert.Equal("JsonValue1", options.JsonKey1); + Assert.Equal("MemValue1", options.MemKey1); + Assert.Equal("XmlValue1", options.XmlKey1); + } + } + public void Dispose() { _fileProvider.Dispose(); @@ -966,5 +1010,18 @@ public void Dispose() await Task.Delay(_msDelay); } } + + private sealed class MyOptions + { + public string CmdKey1 { get; set; } + + public string IniKey1 { get; set; } + + public string JsonKey1 { get; set; } + + public string MemKey1 { get; set; } + + public string XmlKey1 { get; set; } + } } } diff --git a/src/Configuration/test/Config.FunctionalTests/Microsoft.Extensions.Configuration.FunctionalTests.csproj b/src/Configuration/test/Config.FunctionalTests/Microsoft.Extensions.Configuration.FunctionalTests.csproj index 6f7dbe3ddd1..818806cf493 100644 --- a/src/Configuration/test/Config.FunctionalTests/Microsoft.Extensions.Configuration.FunctionalTests.csproj +++ b/src/Configuration/test/Config.FunctionalTests/Microsoft.Extensions.Configuration.FunctionalTests.csproj @@ -9,6 +9,9 @@ + + + From d6e3d4fa1b23d4d016d902a886774fbc982b30ba Mon Sep 17 00:00:00 2001 From: Martin Costello Date: Mon, 4 Mar 2019 16:18:02 +0000 Subject: [PATCH 4/4] Remove OS-specific skip on test File watching is not being used, so remove the OS-specific test case skips. --- .../test/Config.FunctionalTests/ConfigurationTests.cs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/Configuration/test/Config.FunctionalTests/ConfigurationTests.cs b/src/Configuration/test/Config.FunctionalTests/ConfigurationTests.cs index f4d7f0c5df6..95c90ed5caa 100644 --- a/src/Configuration/test/Config.FunctionalTests/ConfigurationTests.cs +++ b/src/Configuration/test/Config.FunctionalTests/ConfigurationTests.cs @@ -944,9 +944,7 @@ public async Task TouchingFileWillReloadForUserSecrets() Assert.True(token.HasChanged); } - [ConditionalFact] - [OSSkipCondition(OperatingSystems.Linux, SkipReason = "File watching is flaky on non windows.")] - [OSSkipCondition(OperatingSystems.MacOSX, SkipReason = "File watching is flaky on non windows.")] + [Fact] public void BindingDoesNotThrowIfReloadedDuringBinding() { WriteTestFiles();