From e5a0275e8e9a949a4575abdecd5a0992b420ff8f Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Thu, 18 May 2023 18:10:22 +0200 Subject: [PATCH] FileConfigurationProvider.Dispose should dispose FileProvider when it ows it fixes #86146 --- .../src/FileConfigurationExtensions.cs | 10 ++- .../src/FileConfigurationProvider.cs | 5 ++ .../src/FileConfigurationSource.cs | 11 ++++ .../tests/JsonConfigurationTest.cs | 61 +++++++++++++++++++ .../tests/XmlConfigurationTest.cs | 61 +++++++++++++++++++ 5 files changed, 142 insertions(+), 6 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.Configuration.FileExtensions/src/FileConfigurationExtensions.cs b/src/libraries/Microsoft.Extensions.Configuration.FileExtensions/src/FileConfigurationExtensions.cs index f84c5c10eea771..77ac387d096077 100644 --- a/src/libraries/Microsoft.Extensions.Configuration.FileExtensions/src/FileConfigurationExtensions.cs +++ b/src/libraries/Microsoft.Extensions.Configuration.FileExtensions/src/FileConfigurationExtensions.cs @@ -29,6 +29,9 @@ public static IConfigurationBuilder SetFileProvider(this IConfigurationBuilder b return builder; } + internal static IFileProvider? GetUserDefinedFileProvider(this IConfigurationBuilder builder) + => builder.Properties.TryGetValue(FileProviderKey, out object? provider) ? (IFileProvider)provider : null; + /// /// Gets the default to be used for file-based providers. /// @@ -38,12 +41,7 @@ public static IFileProvider GetFileProvider(this IConfigurationBuilder builder) { ThrowHelper.ThrowIfNull(builder); - if (builder.Properties.TryGetValue(FileProviderKey, out object? provider)) - { - return (IFileProvider)provider; - } - - return new PhysicalFileProvider(AppContext.BaseDirectory ?? string.Empty); + return GetUserDefinedFileProvider(builder) ?? new PhysicalFileProvider(AppContext.BaseDirectory ?? string.Empty); } /// diff --git a/src/libraries/Microsoft.Extensions.Configuration.FileExtensions/src/FileConfigurationProvider.cs b/src/libraries/Microsoft.Extensions.Configuration.FileExtensions/src/FileConfigurationProvider.cs index d226051b1ab838..c0d8c9f341278a 100644 --- a/src/libraries/Microsoft.Extensions.Configuration.FileExtensions/src/FileConfigurationProvider.cs +++ b/src/libraries/Microsoft.Extensions.Configuration.FileExtensions/src/FileConfigurationProvider.cs @@ -162,6 +162,11 @@ private void HandleException(ExceptionDispatchInfo info) protected virtual void Dispose(bool disposing) { _changeTokenRegistration?.Dispose(); + + if (Source.OwnsFileProvider) + { + (Source.FileProvider as IDisposable)?.Dispose(); + } } } } diff --git a/src/libraries/Microsoft.Extensions.Configuration.FileExtensions/src/FileConfigurationSource.cs b/src/libraries/Microsoft.Extensions.Configuration.FileExtensions/src/FileConfigurationSource.cs index d58c265f406a9a..60555b2c672558 100644 --- a/src/libraries/Microsoft.Extensions.Configuration.FileExtensions/src/FileConfigurationSource.cs +++ b/src/libraries/Microsoft.Extensions.Configuration.FileExtensions/src/FileConfigurationSource.cs @@ -18,6 +18,11 @@ public abstract class FileConfigurationSource : IConfigurationSource /// public IFileProvider? FileProvider { get; set; } + /// + /// Set to true when was not provided by user and can be safely disposed. + /// + internal bool OwnsFileProvider { get; private set; } + /// /// The path to the file. /// @@ -58,6 +63,11 @@ public abstract class FileConfigurationSource : IConfigurationSource /// The . public void EnsureDefaults(IConfigurationBuilder builder) { + if (FileProvider is null && builder.GetUserDefinedFileProvider() is null) + { + OwnsFileProvider = true; + } + FileProvider ??= builder.GetFileProvider(); OnLoadException ??= builder.GetFileLoadExceptionHandler(); } @@ -81,6 +91,7 @@ public void ResolveFileProvider() } if (Directory.Exists(directory)) { + OwnsFileProvider = true; FileProvider = new PhysicalFileProvider(directory); Path = pathToFile; } diff --git a/src/libraries/Microsoft.Extensions.Configuration.Json/tests/JsonConfigurationTest.cs b/src/libraries/Microsoft.Extensions.Configuration.Json/tests/JsonConfigurationTest.cs index 2e3fe00acb2305..4b08c918fb8981 100644 --- a/src/libraries/Microsoft.Extensions.Configuration.Json/tests/JsonConfigurationTest.cs +++ b/src/libraries/Microsoft.Extensions.Configuration.Json/tests/JsonConfigurationTest.cs @@ -4,8 +4,10 @@ using System; using System.Globalization; using System.IO; +using System.Linq; using Microsoft.Extensions.Configuration.Json; using Microsoft.Extensions.Configuration.Test; +using Microsoft.Extensions.FileProviders; using Xunit; namespace Microsoft.Extensions.Configuration @@ -219,5 +221,64 @@ public void ThrowFormatExceptionWhenFileIsEmpty() var exception = Assert.Throws(() => LoadProvider(@"")); Assert.Contains("Could not parse the JSON file.", exception.Message); } + + [Theory] + [InlineData(true)] + [InlineData(false)] + public void AddJsonFile_FileProvider_Gets_Disposed_When_It_Was_Not_Created_By_The_User(bool disposeConfigRoot) + { + string filePath = Path.Combine(Path.GetTempPath(), $"{nameof(AddJsonFile_FileProvider_Gets_Disposed_When_It_Was_Not_Created_By_The_User)}.json"); + File.WriteAllText(filePath, @"{ ""some"": ""value"" }"); + + IConfigurationRoot config = new ConfigurationBuilder().AddJsonFile(filePath, optional: false).Build(); + JsonConfigurationProvider jsonConfigurationProvider = config.Providers.OfType().Single(); + + Assert.NotNull(jsonConfigurationProvider.Source.FileProvider); + PhysicalFileProvider fileProvider = (PhysicalFileProvider)jsonConfigurationProvider.Source.FileProvider; + Assert.False(GetIsDisposed(fileProvider)); + + if (disposeConfigRoot) + { + (config as IDisposable).Dispose(); // disposing ConfigurationRoot + } + else + { + jsonConfigurationProvider.Dispose(); // disposing JsonConfigurationProvider + } + + Assert.True(GetIsDisposed(fileProvider)); + } + + [Fact] + public void AddJsonFile_FileProvider_Is_Not_Disposed_When_It_Is_Owned_By_The_User() + { + string filePath = Path.Combine(Path.GetTempPath(), $"{nameof(AddJsonFile_FileProvider_Is_Not_Disposed_When_It_Is_Owned_By_The_User)}.json"); + File.WriteAllText(filePath, @"{ ""some"": ""value"" }"); + + PhysicalFileProvider fileProvider = new(Path.GetDirectoryName(filePath)); + JsonConfigurationProvider configurationProvider = new(new JsonConfigurationSource() + { + Path = filePath, + FileProvider = fileProvider + }); + IConfigurationRoot config = new ConfigurationBuilder().AddJsonFile(configurationProvider.Source.FileProvider, filePath, optional: true, reloadOnChange: false).Build(); + + Assert.False(GetIsDisposed(fileProvider)); + + (config as IDisposable).Dispose(); // disposing ConfigurationRoot that does not own the provider + Assert.False(GetIsDisposed(fileProvider)); + + configurationProvider.Dispose(); // disposing JsonConfigurationProvider that does not own the provider + Assert.False(GetIsDisposed(fileProvider)); + + fileProvider.Dispose(); // disposing PhysicalFileProvider itself + Assert.True(GetIsDisposed(fileProvider)); + } + + private static bool GetIsDisposed(PhysicalFileProvider fileProvider) + { + System.Reflection.FieldInfo isDisposedField = typeof(PhysicalFileProvider).GetField("_disposed", System.Reflection.BindingFlags.Instance | System.Reflection.BindingFlags.NonPublic); + return (bool)isDisposedField.GetValue(fileProvider); + } } } diff --git a/src/libraries/Microsoft.Extensions.Configuration.Xml/tests/XmlConfigurationTest.cs b/src/libraries/Microsoft.Extensions.Configuration.Xml/tests/XmlConfigurationTest.cs index 4012d775afa5f2..d248d96d9d464f 100644 --- a/src/libraries/Microsoft.Extensions.Configuration.Xml/tests/XmlConfigurationTest.cs +++ b/src/libraries/Microsoft.Extensions.Configuration.Xml/tests/XmlConfigurationTest.cs @@ -3,11 +3,13 @@ using System; using System.IO; +using System.Linq; using System.Security.Cryptography; using System.Security.Cryptography.Xml; using System.Tests; using System.Xml; using Microsoft.Extensions.Configuration.Test; +using Microsoft.Extensions.FileProviders; using Xunit; namespace Microsoft.Extensions.Configuration.Xml.Test @@ -779,5 +781,64 @@ public void LoadKeyValuePairsFromValidEncryptedXml() Assert.Equal("AnotherTestConnectionString", xmlConfigSrc.Get("data.setting:inventory:connectionstring")); Assert.Equal("MySql", xmlConfigSrc.Get("Data.setting:Inventory:Provider")); } + + [Theory] + [InlineData(true)] + [InlineData(false)] + public void AddXmlFile_FileProvider_Gets_Disposed_When_It_Was_Not_Created_By_The_User(bool disposeConfigRoot) + { + string filePath = Path.Combine(Path.GetTempPath(), $"{nameof(AddXmlFile_FileProvider_Gets_Disposed_When_It_Was_Not_Created_By_The_User)}.xml"); + File.WriteAllText(filePath, @"Settings"); + + IConfigurationRoot config = new ConfigurationBuilder().AddXmlFile(filePath, optional: false).Build(); + XmlConfigurationProvider xmlConfigurationProvider = config.Providers.OfType().Single(); + + Assert.NotNull(xmlConfigurationProvider.Source.FileProvider); + PhysicalFileProvider fileProvider = (PhysicalFileProvider)xmlConfigurationProvider.Source.FileProvider; + Assert.False(GetIsDisposed(fileProvider)); + + if (disposeConfigRoot) + { + (config as IDisposable).Dispose(); // disposing ConfigurationRoot + } + else + { + xmlConfigurationProvider.Dispose(); // disposing XmlConfigurationProvider + } + + Assert.True(GetIsDisposed(fileProvider)); + } + + [Fact] + public void AddXmlFile_FileProvider_Is_Not_Disposed_When_It_Is_Owned_By_The_User() + { + string filePath = Path.Combine(Path.GetTempPath(), $"{nameof(AddXmlFile_FileProvider_Is_Not_Disposed_When_It_Is_Owned_By_The_User)}.xml"); + File.WriteAllText(filePath, @"Settings"); + + PhysicalFileProvider fileProvider = new(Path.GetDirectoryName(filePath)); + XmlConfigurationProvider configurationProvider = new(new XmlConfigurationSource() + { + Path = filePath, + FileProvider = fileProvider + }); + IConfigurationRoot config = new ConfigurationBuilder().AddXmlFile(configurationProvider.Source.FileProvider, filePath, optional: true, reloadOnChange: false).Build(); + + Assert.False(GetIsDisposed(fileProvider)); + + (config as IDisposable).Dispose(); // disposing ConfigurationRoot that does not own the provider + Assert.False(GetIsDisposed(fileProvider)); + + configurationProvider.Dispose(); // disposing XmlConfigurationProvider + Assert.False(GetIsDisposed(fileProvider)); + + fileProvider.Dispose(); // disposing PhysicalFileProvider itself + Assert.True(GetIsDisposed(fileProvider)); + } + + private static bool GetIsDisposed(PhysicalFileProvider fileProvider) + { + System.Reflection.FieldInfo isDisposedField = typeof(PhysicalFileProvider).GetField("_disposed", System.Reflection.BindingFlags.Instance | System.Reflection.BindingFlags.NonPublic); + return (bool)isDisposedField.GetValue(fileProvider); + } } }