Skip to content

Commit

Permalink
[release/8.0-staging] Revert "FileConfigurationProvider.Dispose shoul…
Browse files Browse the repository at this point in the history 
…d dispose FileProvider when it owns it" (#101610)

* Revert "FileConfigurationProvider.Dispose should dispose FileProvider when it…"

This reverts commit 63fad3c.

* Add test to ensure the bug does not come back

* Enable servicing Microsoft.Extensions.Configuration.FileExtensions

* Update Microsoft.Extensions.Configuration.FileExtensions.csproj

---------

Co-authored-by: Adam Sitnik <adam.sitnik@gmail.com>
Co-authored-by: Eric StJohn <ericstj@microsoft.com>
  • Loading branch information
@github-actions @adamsitnik @ericstj
3 people committed Apr 30, 2024
1 parent 8ff4dd4 commit 1907984
Show file tree
Hide file tree
Showing 6 changed files with 17 additions and 120 deletions.
10 changes: 6 additions & 4 deletions ...ries/Microsoft.Extensions.Configuration.FileExtensions/src/FileConfigurationExtensions.cs
View file
Expand Up @@ -29,9 +29,6 @@ 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;

/// <summary>
/// Gets the default <see cref="IFileProvider"/> to be used for file-based providers.
/// </summary>
Expand All @@ -41,7 +38,12 @@ public static IFileProvider GetFileProvider(this IConfigurationBuilder builder)
{
ThrowHelper.ThrowIfNull(builder);

return GetUserDefinedFileProvider(builder) ?? new PhysicalFileProvider(AppContext.BaseDirectory ?? string.Empty);
if (builder.Properties.TryGetValue(FileProviderKey, out object? provider))
{
return (IFileProvider)provider;
}

return new PhysicalFileProvider(AppContext.BaseDirectory ?? string.Empty);
}

/// <summary>
Expand Down
5 changes: 0 additions & 5 deletions ...raries/Microsoft.Extensions.Configuration.FileExtensions/src/FileConfigurationProvider.cs
View file
Expand Up @@ -162,11 +162,6 @@ private void HandleException(ExceptionDispatchInfo info)
protected virtual void Dispose(bool disposing)
{
_changeTokenRegistration?.Dispose();

if (Source.OwnsFileProvider)
{
(Source.FileProvider as IDisposable)?.Dispose();
}
}
}
}
11 changes: 0 additions & 11 deletions ...ibraries/Microsoft.Extensions.Configuration.FileExtensions/src/FileConfigurationSource.cs
View file
Expand Up @@ -18,11 +18,6 @@ public abstract class FileConfigurationSource : IConfigurationSource
/// </summary>
public IFileProvider? FileProvider { get; set; }

/// <summary>
/// Set to true when <see cref="FileProvider"/> was not provided by user and can be safely disposed.
/// </summary>
internal bool OwnsFileProvider { get; private set; }

/// <summary>
/// The path to the file.
/// </summary>
Expand Down Expand Up @@ -63,11 +58,6 @@ public abstract class FileConfigurationSource : IConfigurationSource
/// <param name="builder">The <see cref="IConfigurationBuilder"/>.</param>
public void EnsureDefaults(IConfigurationBuilder builder)
{
if (FileProvider is null && builder.GetUserDefinedFileProvider() is null)
{
OwnsFileProvider = true;
}

FileProvider ??= builder.GetFileProvider();
OnLoadException ??= builder.GetFileLoadExceptionHandler();
}
Expand All @@ -91,7 +81,6 @@ public void ResolveFileProvider()
}
if (Directory.Exists(directory))
{
OwnsFileProvider = true;
FileProvider = new PhysicalFileProvider(directory);
Path = pathToFile;
}
Expand Down
2 changes: 2 additions & 0 deletions ...Configuration.FileExtensions/src/Microsoft.Extensions.Configuration.FileExtensions.csproj
View file
Expand Up @@ -4,6 +4,8 @@
<TargetFrameworks>$(NetCoreAppCurrent);$(NetCoreAppPrevious);$(NetCoreAppMinimum);netstandard2.0;$(NetFrameworkMinimum)</TargetFrameworks>
<EnableDefaultItems>true</EnableDefaultItems>
<IsPackable>true</IsPackable>
<GeneratePackageOnBuild>true</GeneratePackageOnBuild>
<ServicingVersion>1</ServicingVersion>
<PackageDescription>Provides a base class for file-based configuration providers used with Microsoft.Extensions.Configuration and extension methods for configuring them.</PackageDescription>
</PropertyGroup>

Expand Down
48 changes: 9 additions & 39 deletions src/libraries/Microsoft.Extensions.Configuration.Json/tests/JsonConfigurationTest.cs
View file
Expand Up @@ -222,56 +222,26 @@ public void ThrowFormatExceptionWhenFileIsEmpty()
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)
[Fact]
public void AddJsonFile_FileProvider_Is_Not_Disposed_When_SourcesGetReloaded()
{
string filePath = Path.Combine(Path.GetTempPath(), $"{nameof(AddJsonFile_FileProvider_Gets_Disposed_When_It_Was_Not_Created_By_The_User)}.json");
string filePath = Path.Combine(Path.GetTempPath(), $"{nameof(AddJsonFile_FileProvider_Is_Not_Disposed_When_SourcesGetReloaded)}.json");
File.WriteAllText(filePath, @"{ ""some"": ""value"" }");

IConfigurationRoot config = new ConfigurationBuilder().AddJsonFile(filePath, optional: false).Build();
JsonConfigurationProvider jsonConfigurationProvider = config.Providers.OfType<JsonConfigurationProvider>().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));
}
IConfigurationBuilder builder = new ConfigurationManager();

[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"" }");
builder.AddJsonFile(filePath, optional: false);

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();
FileConfigurationSource fileConfigurationSource = (FileConfigurationSource)builder.Sources.Last();
PhysicalFileProvider fileProvider = (PhysicalFileProvider)fileConfigurationSource.FileProvider;

Assert.False(GetIsDisposed(fileProvider));

(config as IDisposable).Dispose(); // disposing ConfigurationRoot that does not own the provider
Assert.False(GetIsDisposed(fileProvider));
builder.Properties.Add("simplest", "repro");

configurationProvider.Dispose(); // disposing JsonConfigurationProvider that does not own the provider
Assert.False(GetIsDisposed(fileProvider));

fileProvider.Dispose(); // disposing PhysicalFileProvider itself
fileProvider.Dispose();
Assert.True(GetIsDisposed(fileProvider));
}

Expand Down
61 changes: 0 additions & 61 deletions src/libraries/Microsoft.Extensions.Configuration.Xml/tests/XmlConfigurationTest.cs
View file
Expand Up @@ -3,13 +3,11 @@

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
Expand Down Expand Up @@ -781,64 +779,5 @@ 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><My><Nice>Settings</Nice></My></settings>");

IConfigurationRoot config = new ConfigurationBuilder().AddXmlFile(filePath, optional: false).Build();
XmlConfigurationProvider xmlConfigurationProvider = config.Providers.OfType<XmlConfigurationProvider>().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><My><Nice>Settings</Nice></My></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);
}
}
}

0 comments on commit 1907984

Please sign in to comment.