Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[release/8.0-staging] Revert "FileConfigurationProvider.Dispose should dispose FileProvider when it owns it" #101610

Merged
merged 4 commits into from Apr 30, 2024
Merged

[release/8.0-staging] Revert "FileConfigurationProvider.Dispose should dispose FileProvider when it owns it" #101610

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b6d2144
Revert "FileConfigurationProvider.Dispose should dispose FileProvider…
adamsitnik Apr 26, 2024
e84db1f
Add test to ensure the bug does not come back
adamsitnik Apr 26, 2024
9dffd8e
Enable servicing Microsoft.Extensions.Configuration.FileExtensions
ericstj Apr 29, 2024
c5e4269
Update Microsoft.Extensions.Configuration.FileExtensions.csproj
ericstj Apr 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 6 additions & 4 deletions ...ries/Microsoft.Extensions.Configuration.FileExtensions/src/FileConfigurationExtensions.cs
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
48 changes: 9 additions & 39 deletions src/libraries/Microsoft.Extensions.Configuration.Json/tests/JsonConfigurationTest.cs
View file
Open in desktop
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
Open in desktop
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);
}
}
}