From 192abfdf3e73106e40d7651eecfb621e4f78c344 Mon Sep 17 00:00:00 2001 From: Martin Costello Date: Mon, 4 Mar 2019 18:14:16 +0000 Subject: [PATCH] Atomically swap config data (#1202) 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 collection being modified. Relates to #1189. --- ...vironmentVariablesConfigurationProvider.cs | 6 +- .../test/EnvironmentVariablesTest.cs | 47 +++++++++++++++ .../src/KeyPerFileConfigurationProvider.cs | 7 ++- .../Config.KeyPerFile/test/KeyPerFileTests.cs | 43 ++++++++++++++ .../ConfigurationTests.cs | 59 ++++++++++++++++++- ...sions.Configuration.FunctionalTests.csproj | 3 + 6 files changed, 159 insertions(+), 6 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.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/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() 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 diff --git a/src/Configuration/test/Config.FunctionalTests/ConfigurationTests.cs b/src/Configuration/test/Config.FunctionalTests/ConfigurationTests.cs index 27c694c43be..95c90ed5caa 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,48 @@ public async Task TouchingFileWillReloadForUserSecrets() Assert.True(token.HasChanged); } + [Fact] + 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 +1008,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 @@ + + +