Reloading sources in ConfigurationManager should not dispose IConfigurationSources, only providers

[adamsitnik]

This PR fixes#95745 with low risk (to allow backporting), but in an ugly way as the existing public APIs don't allow for a proper fix and introducing a new API would require breaking changes.

[ericstj]

Is this a safe assumption? Can't any FileConfigurationSource have Build called more than once?

Also, I've noticed others implement IConfigurationBuilder before. Some are forks of ConfigurationManager, others are unique. I can share some data offline.

A couple other options to consider:

1. Just create a new FileProvider if the previous one is disposed. The logic for creating a new one isn't specific to the provider (it's implemented in an extension method we own IIRC).
2. Share a single FIleProvider for default and don't dispose it. We still leak until finalization, but only one is leaked.
3. Implement ref-counting, or disposal pattern for the source. If we just add IDispose to the source impl, will it get called by whichever component manages it's lifetime today?

[adamsitnik]

@ericstj Thank you for feedback! A lot of good questions! Here are my findings:

Is this a safe assumption? Can't any FileConfigurationSource have Build called more than once?

ConfigurationManager can call it more than once. It's called by ReloadSources which is called... anytime a source is being deleted, inserted or any property changes

Also, I've noticed others implement IConfigurationBuilder before. Some are forks of ConfigurationManager, others are unique.

ConfigurationManager is sealed, I could change this condition from to check for explicit type.

runtime/src/libraries/Microsoft.Extensions.Configuration/src/ConfigurationManager.cs

Line 25 in a971763

publicsealedclassConfigurationManager:IConfigurationManager,IConfigurationRoot,IDisposable

[adamsitnik]

Just create a new FileProvider if the previous one is disposed.

I was thinking about it. The idea I had was to null the FileProvider after disposing it and let ReloadSources re-assign it:

runtime/src/libraries/Microsoft.Extensions.Configuration/src/ConfigurationManager.cs

Line 146 in a971763

newProvidersList.Add(source.Build(this));

runtime/src/libraries/Microsoft.Extensions.Configuration.Json/src/JsonConfigurationSource.cs

Lines 18 to 20 in 1a7904e

	publicoverrideIConfigurationProviderBuild(IConfigurationBuilderbuilder)
	{
	EnsureDefaults(builder);

runtime/src/libraries/Microsoft.Extensions.Configuration.FileExtensions/src/FileConfigurationSource.cs

Line 71 in 1a7904e

FileProvider??=builder.GetFileProvider();

But then I've realized that ReloadSources creates new providers firsts, then tries to replace (it needs new ones to get rid of old ones) them:

runtime/src/libraries/Microsoft.Extensions.Configuration/src/ConfigurationManager.cs

Lines 136 to 157 in a971763

	privatevoidReloadSources()
	{
	DisposeRegistrations();
	_changeTokenRegistrations.Clear();
	varnewProvidersList=newList<IConfigurationProvider>();
	foreach(IConfigurationSourcesourcein_sources)
	{
	newProvidersList.Add(source.Build(this));
	}
	foreach(IConfigurationProviderpinnewProvidersList)
	{
	p.Load();
	_changeTokenRegistrations.Add(ChangeToken.OnChange(p.GetReloadToken,RaiseChanged));
	}
	_providerManager.ReplaceProviders(newProvidersList);
	RaiseChanged();
	}

and it tries to do that in thread-safe way:

runtime/src/libraries/Microsoft.Extensions.Configuration/src/ReferenceCountedProvidersManager.cs

Lines 41 to 58 in a971763

	publicvoidReplaceProviders(List<IConfigurationProvider>providers)
	{
	ReferenceCountedProvidersoldRefCountedProviders=_refCountedProviders;
	lock(_replaceProvidersLock)
	{
	if(_disposed)
	{
	thrownewObjectDisposedException(nameof(ConfigurationManager));
	}
	_refCountedProviders=ReferenceCountedProviders.Create(providers);
	}
	// Decrement the reference count to the old providers. If they are being concurrently read from
	// the actual disposal of the old providers will be delayed until the final reference is released.
	// Never dispose ReferenceCountedProviders with a lock because this may call into user code.
	oldRefCountedProviders.Dispose();

Changing the order would most likely be possible, but I don't see a possibility to do it in a simple way with low risk (I was searching for a fix that could be backported to 8.0)

[adamsitnik]

Share a single FIleProvider for default and don't dispose it. We still leak until finalization, but only one is leaked.

This is something I've not considered. How would it affect perf on various OSes? Is it better to have a single instance of PhysicalFilesWatcher rather than many?

runtime/src/libraries/Microsoft.Extensions.FileProviders.Physical/src/PhysicalFileProvider.cs

Line 32 in 1a7904e

privatePhysicalFilesWatcher?_fileWatcher;

Currently a new instance if being allocated if the user has not configured a default one (but I don't know whether for example ASP.NET does not do that somewhere else):

runtime/src/libraries/Microsoft.Extensions.Configuration.FileExtensions/src/FileConfigurationExtensions.cs

Line 44 in a971763

returnGetUserDefinedFileProvider(builder)??newPhysicalFileProvider(AppContext.BaseDirectory??string.Empty);

[adamsitnik]

Implement ref-counting

I am not a big fan of ref-counting (it's hardly ever implemented properly and always brings a lot of complexity).

or disposal pattern for the source. If we just add IDispose to the source impl, will it get called by whichever component manages it's lifetime today

From my perspective, the main issue is that ConfigurationRoot is given with a list of providers, but it does not have reference to the sources. So it can not dispose the sources with the current public API.

runtime/src/libraries/Microsoft.Extensions.Configuration/src/ConfigurationBuilder.cs

Lines 47 to 53 in a971763

	varproviders=newList<IConfigurationProvider>();
	foreach(IConfigurationSourcesourcein_sources)
	{
	IConfigurationProviderprovider=source.Build(this);
	providers.Add(provider);
	}
	returnnewConfigurationRoot(providers);

runtime/src/libraries/Microsoft.Extensions.Configuration/src/ConfigurationRoot.cs

Lines 99 to 112 in a971763

	publicvoidDispose()
	{
	// dispose change token registrations
	foreach(IDisposableregistrationin_changeTokenRegistrations)
	{
	registration.Dispose();
	}
	// dispose providers
	foreach(IConfigurationProviderproviderin_providers)
	{
	(providerasIDisposable)?.Dispose();
	}
	}

ConfigurationManager has both and can dispose either one or both of them.

In #100642 I've extended the provider abstraction with a reference to source (most of the implementations already implement a property that returns the source). It allows for disposing what is needed. But introducing a new public API would be a breaking change (so it can not be backported).

[ericstj]

Is this a safe assumption? Can't any FileConfigurationSource have Build called more than once?
...

ConfigurationManager is sealed, I could change this condition from to check for explicit type.

My point was that special casing ConfigurationManager isn't really a fix. Anyone can call

IConfigurationSourcesource=newDerivedFileConfigurationSource();using(IConfigurationProviderprovider=source.Build(builder)){}using(IConfigurationProviderprovider=source.Build(builder)){}

I believe it's intentional that Sources can produce multiple providers. So we really shouldn't assume that the provider can dispose resources held by the source.

Just create a new FileProvider if the previous one is disposed.
...
Changing the order would most likely be possible, but I don't see a possibility to do it in a simple way with low risk (I was searching for a fix that could be backported to 8.0)

Maybe you don't need to change the order - if your design is to delegate the lifetime of the FileProvider to the IConfigurationProvider then you should really be creating a new FileProvider for each call to Build. I think this is the solution that's most consistent with the change you made.

Share a single FIleProvider for default and don't dispose it. We still leak until finalization, but only one is leaked.
This is something I've not considered. How would it affect perf on various OSes? Is it better to have a single instance of PhysicalFilesWatcher rather than many?

I think it's a tradeoff (but @jozkee might have a better idea as he's our resident FSW expert). I think some systems have expense per-watcher like a background thread and kernel buffers - and those can approach a limit - so more of them uses more resources or could hit the limit - thus the whole polling watcher. From the same token those kernel buffers associated with a watcher limit how many events it can buffer between callbacks - so giving more work to a single one could result in events lost. At least for servicing - let's try to keep the same number of watchers as before. Folks can always set the provider if they want to experiment with a single FSW for perf. If you can see a better option for vNext we can consider that.

Agreed that RefCounting is too big and complex. I think a clear assignment of responsibility of lifetime for the file-provider is what we want. If it's possible to transfer the responsibility to the source and have the source's lifetime managed by something we could do that in the future. I don't think we should make the provider responsible for lifetime of resources held by the source - this regression made it clear that their lifetimes (and instance counts) differ.

[ericstj]

Bumping this @adamsitnik

[adamsitnik]

This PR fixes#95745 with low risk (to allow backporting), but in an ugly way as the existing public APIs don't allow for a proper fix and introducing a new API would require breaking changes.

[ericstj]

Is this a safe assumption? Can't any FileConfigurationSource have Build called more than once?

Also, I've noticed others implement IConfigurationBuilder before. Some are forks of ConfigurationManager, others are unique. I can share some data offline.

A couple other options to consider:

1. Just create a new FileProvider if the previous one is disposed. The logic for creating a new one isn't specific to the provider (it's implemented in an extension method we own IIRC).
2. Share a single FIleProvider for default and don't dispose it. We still leak until finalization, but only one is leaked.
3. Implement ref-counting, or disposal pattern for the source. If we just add IDispose to the source impl, will it get called by whichever component manages it's lifetime today?

[adamsitnik]

@ericstj Thank you for feedback! A lot of good questions! Here are my findings:

Is this a safe assumption? Can't any FileConfigurationSource have Build called more than once?

ConfigurationManager can call it more than once. It's called by ReloadSources which is called... anytime a source is being deleted, inserted or any property changes

Also, I've noticed others implement IConfigurationBuilder before. Some are forks of ConfigurationManager, others are unique.

ConfigurationManager is sealed, I could change this condition from to check for explicit type.

runtime/src/libraries/Microsoft.Extensions.Configuration/src/ConfigurationManager.cs

Line 25 in a971763

publicsealedclassConfigurationManager:IConfigurationManager,IConfigurationRoot,IDisposable

[adamsitnik]

Just create a new FileProvider if the previous one is disposed.

I was thinking about it. The idea I had was to null the FileProvider after disposing it and let ReloadSources re-assign it:

runtime/src/libraries/Microsoft.Extensions.Configuration/src/ConfigurationManager.cs

Line 146 in a971763

newProvidersList.Add(source.Build(this));

runtime/src/libraries/Microsoft.Extensions.Configuration.Json/src/JsonConfigurationSource.cs

Lines 18 to 20 in 1a7904e

	publicoverrideIConfigurationProviderBuild(IConfigurationBuilderbuilder)
	{
	EnsureDefaults(builder);

runtime/src/libraries/Microsoft.Extensions.Configuration.FileExtensions/src/FileConfigurationSource.cs

Line 71 in 1a7904e

FileProvider??=builder.GetFileProvider();

But then I've realized that ReloadSources creates new providers firsts, then tries to replace (it needs new ones to get rid of old ones) them:

runtime/src/libraries/Microsoft.Extensions.Configuration/src/ConfigurationManager.cs

Lines 136 to 157 in a971763

	privatevoidReloadSources()
	{
	DisposeRegistrations();
	_changeTokenRegistrations.Clear();
	varnewProvidersList=newList<IConfigurationProvider>();
	foreach(IConfigurationSourcesourcein_sources)
	{
	newProvidersList.Add(source.Build(this));
	}
	foreach(IConfigurationProviderpinnewProvidersList)
	{
	p.Load();
	_changeTokenRegistrations.Add(ChangeToken.OnChange(p.GetReloadToken,RaiseChanged));
	}
	_providerManager.ReplaceProviders(newProvidersList);
	RaiseChanged();
	}

and it tries to do that in thread-safe way:

runtime/src/libraries/Microsoft.Extensions.Configuration/src/ReferenceCountedProvidersManager.cs

Lines 41 to 58 in a971763

	publicvoidReplaceProviders(List<IConfigurationProvider>providers)
	{
	ReferenceCountedProvidersoldRefCountedProviders=_refCountedProviders;
	lock(_replaceProvidersLock)
	{
	if(_disposed)
	{
	thrownewObjectDisposedException(nameof(ConfigurationManager));
	}
	_refCountedProviders=ReferenceCountedProviders.Create(providers);
	}
	// Decrement the reference count to the old providers. If they are being concurrently read from
	// the actual disposal of the old providers will be delayed until the final reference is released.
	// Never dispose ReferenceCountedProviders with a lock because this may call into user code.
	oldRefCountedProviders.Dispose();

Changing the order would most likely be possible, but I don't see a possibility to do it in a simple way with low risk (I was searching for a fix that could be backported to 8.0)

[adamsitnik]

Share a single FIleProvider for default and don't dispose it. We still leak until finalization, but only one is leaked.

This is something I've not considered. How would it affect perf on various OSes? Is it better to have a single instance of PhysicalFilesWatcher rather than many?

runtime/src/libraries/Microsoft.Extensions.FileProviders.Physical/src/PhysicalFileProvider.cs

Line 32 in 1a7904e

privatePhysicalFilesWatcher?_fileWatcher;

Currently a new instance if being allocated if the user has not configured a default one (but I don't know whether for example ASP.NET does not do that somewhere else):

runtime/src/libraries/Microsoft.Extensions.Configuration.FileExtensions/src/FileConfigurationExtensions.cs

Line 44 in a971763

returnGetUserDefinedFileProvider(builder)??newPhysicalFileProvider(AppContext.BaseDirectory??string.Empty);

[adamsitnik]

Implement ref-counting

I am not a big fan of ref-counting (it's hardly ever implemented properly and always brings a lot of complexity).

or disposal pattern for the source. If we just add IDispose to the source impl, will it get called by whichever component manages it's lifetime today

From my perspective, the main issue is that ConfigurationRoot is given with a list of providers, but it does not have reference to the sources. So it can not dispose the sources with the current public API.

runtime/src/libraries/Microsoft.Extensions.Configuration/src/ConfigurationBuilder.cs

Lines 47 to 53 in a971763

	varproviders=newList<IConfigurationProvider>();
	foreach(IConfigurationSourcesourcein_sources)
	{
	IConfigurationProviderprovider=source.Build(this);
	providers.Add(provider);
	}
	returnnewConfigurationRoot(providers);

runtime/src/libraries/Microsoft.Extensions.Configuration/src/ConfigurationRoot.cs

Lines 99 to 112 in a971763

	publicvoidDispose()
	{
	// dispose change token registrations
	foreach(IDisposableregistrationin_changeTokenRegistrations)
	{
	registration.Dispose();
	}
	// dispose providers
	foreach(IConfigurationProviderproviderin_providers)
	{
	(providerasIDisposable)?.Dispose();
	}
	}

ConfigurationManager has both and can dispose either one or both of them.

In #100642 I've extended the provider abstraction with a reference to source (most of the implementations already implement a property that returns the source). It allows for disposing what is needed. But introducing a new public API would be a breaking change (so it can not be backported).

[ericstj]

Is this a safe assumption? Can't any FileConfigurationSource have Build called more than once?
...

ConfigurationManager is sealed, I could change this condition from to check for explicit type.

My point was that special casing ConfigurationManager isn't really a fix. Anyone can call

IConfigurationSourcesource=newDerivedFileConfigurationSource();using(IConfigurationProviderprovider=source.Build(builder)){}using(IConfigurationProviderprovider=source.Build(builder)){}

I believe it's intentional that Sources can produce multiple providers. So we really shouldn't assume that the provider can dispose resources held by the source.

Just create a new FileProvider if the previous one is disposed.
...
Changing the order would most likely be possible, but I don't see a possibility to do it in a simple way with low risk (I was searching for a fix that could be backported to 8.0)

Maybe you don't need to change the order - if your design is to delegate the lifetime of the FileProvider to the IConfigurationProvider then you should really be creating a new FileProvider for each call to Build. I think this is the solution that's most consistent with the change you made.

Share a single FIleProvider for default and don't dispose it. We still leak until finalization, but only one is leaked.
This is something I've not considered. How would it affect perf on various OSes? Is it better to have a single instance of PhysicalFilesWatcher rather than many?

I think it's a tradeoff (but @jozkee might have a better idea as he's our resident FSW expert). I think some systems have expense per-watcher like a background thread and kernel buffers - and those can approach a limit - so more of them uses more resources or could hit the limit - thus the whole polling watcher. From the same token those kernel buffers associated with a watcher limit how many events it can buffer between callbacks - so giving more work to a single one could result in events lost. At least for servicing - let's try to keep the same number of watchers as before. Folks can always set the provider if they want to experiment with a single FSW for perf. If you can see a better option for vNext we can consider that.

Agreed that RefCounting is too big and complex. I think a clear assignment of responsibility of lifetime for the file-provider is what we want. If it's possible to transfer the responsibility to the source and have the source's lifetime managed by something we could do that in the future. I don't think we should make the provider responsible for lifetime of resources held by the source - this regression made it clear that their lifetimes (and instance counts) differ.

[ericstj]

Bumping this @adamsitnik

[adamsitnik]

This PR fixes#95745 with low risk (to allow backporting), but in an ugly way as the existing public APIs don't allow for a proper fix and introducing a new API would require breaking changes.

[ericstj]

Is this a safe assumption? Can't any FileConfigurationSource have Build called more than once?

Also, I've noticed others implement IConfigurationBuilder before. Some are forks of ConfigurationManager, others are unique. I can share some data offline.

A couple other options to consider:

1. Just create a new FileProvider if the previous one is disposed. The logic for creating a new one isn't specific to the provider (it's implemented in an extension method we own IIRC).
2. Share a single FIleProvider for default and don't dispose it. We still leak until finalization, but only one is leaked.
3. Implement ref-counting, or disposal pattern for the source. If we just add IDispose to the source impl, will it get called by whichever component manages it's lifetime today?

[adamsitnik]

@ericstj Thank you for feedback! A lot of good questions! Here are my findings:

Is this a safe assumption? Can't any FileConfigurationSource have Build called more than once?

ConfigurationManager can call it more than once. It's called by ReloadSources which is called... anytime a source is being deleted, inserted or any property changes

Also, I've noticed others implement IConfigurationBuilder before. Some are forks of ConfigurationManager, others are unique.

ConfigurationManager is sealed, I could change this condition from to check for explicit type.

runtime/src/libraries/Microsoft.Extensions.Configuration/src/ConfigurationManager.cs

Line 25 in a971763

publicsealedclassConfigurationManager:IConfigurationManager,IConfigurationRoot,IDisposable

[adamsitnik]

Just create a new FileProvider if the previous one is disposed.

I was thinking about it. The idea I had was to null the FileProvider after disposing it and let ReloadSources re-assign it:

runtime/src/libraries/Microsoft.Extensions.Configuration/src/ConfigurationManager.cs

Line 146 in a971763

newProvidersList.Add(source.Build(this));

runtime/src/libraries/Microsoft.Extensions.Configuration.Json/src/JsonConfigurationSource.cs

Lines 18 to 20 in 1a7904e

	publicoverrideIConfigurationProviderBuild(IConfigurationBuilderbuilder)
	{
	EnsureDefaults(builder);

runtime/src/libraries/Microsoft.Extensions.Configuration.FileExtensions/src/FileConfigurationSource.cs

Line 71 in 1a7904e

FileProvider??=builder.GetFileProvider();

But then I've realized that ReloadSources creates new providers firsts, then tries to replace (it needs new ones to get rid of old ones) them:

runtime/src/libraries/Microsoft.Extensions.Configuration/src/ConfigurationManager.cs

Lines 136 to 157 in a971763

	privatevoidReloadSources()
	{
	DisposeRegistrations();
	_changeTokenRegistrations.Clear();
	varnewProvidersList=newList<IConfigurationProvider>();
	foreach(IConfigurationSourcesourcein_sources)
	{
	newProvidersList.Add(source.Build(this));
	}
	foreach(IConfigurationProviderpinnewProvidersList)
	{
	p.Load();
	_changeTokenRegistrations.Add(ChangeToken.OnChange(p.GetReloadToken,RaiseChanged));
	}
	_providerManager.ReplaceProviders(newProvidersList);
	RaiseChanged();
	}

and it tries to do that in thread-safe way:

runtime/src/libraries/Microsoft.Extensions.Configuration/src/ReferenceCountedProvidersManager.cs

Lines 41 to 58 in a971763

	publicvoidReplaceProviders(List<IConfigurationProvider>providers)
	{
	ReferenceCountedProvidersoldRefCountedProviders=_refCountedProviders;
	lock(_replaceProvidersLock)
	{
	if(_disposed)
	{
	thrownewObjectDisposedException(nameof(ConfigurationManager));
	}
	_refCountedProviders=ReferenceCountedProviders.Create(providers);
	}
	// Decrement the reference count to the old providers. If they are being concurrently read from
	// the actual disposal of the old providers will be delayed until the final reference is released.
	// Never dispose ReferenceCountedProviders with a lock because this may call into user code.
	oldRefCountedProviders.Dispose();

Changing the order would most likely be possible, but I don't see a possibility to do it in a simple way with low risk (I was searching for a fix that could be backported to 8.0)

[adamsitnik]

Share a single FIleProvider for default and don't dispose it. We still leak until finalization, but only one is leaked.

This is something I've not considered. How would it affect perf on various OSes? Is it better to have a single instance of PhysicalFilesWatcher rather than many?

runtime/src/libraries/Microsoft.Extensions.FileProviders.Physical/src/PhysicalFileProvider.cs

Line 32 in 1a7904e

privatePhysicalFilesWatcher?_fileWatcher;

Currently a new instance if being allocated if the user has not configured a default one (but I don't know whether for example ASP.NET does not do that somewhere else):

runtime/src/libraries/Microsoft.Extensions.Configuration.FileExtensions/src/FileConfigurationExtensions.cs

Line 44 in a971763

returnGetUserDefinedFileProvider(builder)??newPhysicalFileProvider(AppContext.BaseDirectory??string.Empty);

[adamsitnik]

Implement ref-counting

I am not a big fan of ref-counting (it's hardly ever implemented properly and always brings a lot of complexity).

or disposal pattern for the source. If we just add IDispose to the source impl, will it get called by whichever component manages it's lifetime today

From my perspective, the main issue is that ConfigurationRoot is given with a list of providers, but it does not have reference to the sources. So it can not dispose the sources with the current public API.

runtime/src/libraries/Microsoft.Extensions.Configuration/src/ConfigurationBuilder.cs

Lines 47 to 53 in a971763

	varproviders=newList<IConfigurationProvider>();
	foreach(IConfigurationSourcesourcein_sources)
	{
	IConfigurationProviderprovider=source.Build(this);
	providers.Add(provider);
	}
	returnnewConfigurationRoot(providers);

runtime/src/libraries/Microsoft.Extensions.Configuration/src/ConfigurationRoot.cs

Lines 99 to 112 in a971763

	publicvoidDispose()
	{
	// dispose change token registrations
	foreach(IDisposableregistrationin_changeTokenRegistrations)
	{
	registration.Dispose();
	}
	// dispose providers
	foreach(IConfigurationProviderproviderin_providers)
	{
	(providerasIDisposable)?.Dispose();
	}
	}

ConfigurationManager has both and can dispose either one or both of them.

In #100642 I've extended the provider abstraction with a reference to source (most of the implementations already implement a property that returns the source). It allows for disposing what is needed. But introducing a new public API would be a breaking change (so it can not be backported).

[ericstj]

Is this a safe assumption? Can't any FileConfigurationSource have Build called more than once?
...

ConfigurationManager is sealed, I could change this condition from to check for explicit type.

My point was that special casing ConfigurationManager isn't really a fix. Anyone can call

IConfigurationSourcesource=newDerivedFileConfigurationSource();using(IConfigurationProviderprovider=source.Build(builder)){}using(IConfigurationProviderprovider=source.Build(builder)){}

I believe it's intentional that Sources can produce multiple providers. So we really shouldn't assume that the provider can dispose resources held by the source.

Just create a new FileProvider if the previous one is disposed.
...
Changing the order would most likely be possible, but I don't see a possibility to do it in a simple way with low risk (I was searching for a fix that could be backported to 8.0)

Maybe you don't need to change the order - if your design is to delegate the lifetime of the FileProvider to the IConfigurationProvider then you should really be creating a new FileProvider for each call to Build. I think this is the solution that's most consistent with the change you made.

Share a single FIleProvider for default and don't dispose it. We still leak until finalization, but only one is leaked.
This is something I've not considered. How would it affect perf on various OSes? Is it better to have a single instance of PhysicalFilesWatcher rather than many?

I think it's a tradeoff (but @jozkee might have a better idea as he's our resident FSW expert). I think some systems have expense per-watcher like a background thread and kernel buffers - and those can approach a limit - so more of them uses more resources or could hit the limit - thus the whole polling watcher. From the same token those kernel buffers associated with a watcher limit how many events it can buffer between callbacks - so giving more work to a single one could result in events lost. At least for servicing - let's try to keep the same number of watchers as before. Folks can always set the provider if they want to experiment with a single FSW for perf. If you can see a better option for vNext we can consider that.

Agreed that RefCounting is too big and complex. I think a clear assignment of responsibility of lifetime for the file-provider is what we want. If it's possible to transfer the responsibility to the source and have the source's lifetime managed by something we could do that in the future. I don't think we should make the provider responsible for lifetime of resources held by the source - this regression made it clear that their lifetimes (and instance counts) differ.

[ericstj]

Bumping this @adamsitnik

[adamsitnik]

This PR fixes#95745 with low risk (to allow backporting), but in an ugly way as the existing public APIs don't allow for a proper fix and introducing a new API would require breaking changes.

[ericstj]

Is this a safe assumption? Can't any FileConfigurationSource have Build called more than once?

Also, I've noticed others implement IConfigurationBuilder before. Some are forks of ConfigurationManager, others are unique. I can share some data offline.

A couple other options to consider:

1. Just create a new FileProvider if the previous one is disposed. The logic for creating a new one isn't specific to the provider (it's implemented in an extension method we own IIRC).
2. Share a single FIleProvider for default and don't dispose it. We still leak until finalization, but only one is leaked.
3. Implement ref-counting, or disposal pattern for the source. If we just add IDispose to the source impl, will it get called by whichever component manages it's lifetime today?

[adamsitnik]

@ericstj Thank you for feedback! A lot of good questions! Here are my findings:

Is this a safe assumption? Can't any FileConfigurationSource have Build called more than once?

ConfigurationManager can call it more than once. It's called by ReloadSources which is called... anytime a source is being deleted, inserted or any property changes

Also, I've noticed others implement IConfigurationBuilder before. Some are forks of ConfigurationManager, others are unique.

ConfigurationManager is sealed, I could change this condition from to check for explicit type.

runtime/src/libraries/Microsoft.Extensions.Configuration/src/ConfigurationManager.cs

Line 25 in a971763

publicsealedclassConfigurationManager:IConfigurationManager,IConfigurationRoot,IDisposable

[adamsitnik]

Just create a new FileProvider if the previous one is disposed.

I was thinking about it. The idea I had was to null the FileProvider after disposing it and let ReloadSources re-assign it:

runtime/src/libraries/Microsoft.Extensions.Configuration/src/ConfigurationManager.cs

Line 146 in a971763

newProvidersList.Add(source.Build(this));

runtime/src/libraries/Microsoft.Extensions.Configuration.Json/src/JsonConfigurationSource.cs

Lines 18 to 20 in 1a7904e

	publicoverrideIConfigurationProviderBuild(IConfigurationBuilderbuilder)
	{
	EnsureDefaults(builder);

runtime/src/libraries/Microsoft.Extensions.Configuration.FileExtensions/src/FileConfigurationSource.cs

Line 71 in 1a7904e

FileProvider??=builder.GetFileProvider();

But then I've realized that ReloadSources creates new providers firsts, then tries to replace (it needs new ones to get rid of old ones) them:

runtime/src/libraries/Microsoft.Extensions.Configuration/src/ConfigurationManager.cs

Lines 136 to 157 in a971763

	privatevoidReloadSources()
	{
	DisposeRegistrations();
	_changeTokenRegistrations.Clear();
	varnewProvidersList=newList<IConfigurationProvider>();
	foreach(IConfigurationSourcesourcein_sources)
	{
	newProvidersList.Add(source.Build(this));
	}
	foreach(IConfigurationProviderpinnewProvidersList)
	{
	p.Load();
	_changeTokenRegistrations.Add(ChangeToken.OnChange(p.GetReloadToken,RaiseChanged));
	}
	_providerManager.ReplaceProviders(newProvidersList);
	RaiseChanged();
	}

and it tries to do that in thread-safe way:

runtime/src/libraries/Microsoft.Extensions.Configuration/src/ReferenceCountedProvidersManager.cs

Lines 41 to 58 in a971763

	publicvoidReplaceProviders(List<IConfigurationProvider>providers)
	{
	ReferenceCountedProvidersoldRefCountedProviders=_refCountedProviders;
	lock(_replaceProvidersLock)
	{
	if(_disposed)
	{
	thrownewObjectDisposedException(nameof(ConfigurationManager));
	}
	_refCountedProviders=ReferenceCountedProviders.Create(providers);
	}
	// Decrement the reference count to the old providers. If they are being concurrently read from
	// the actual disposal of the old providers will be delayed until the final reference is released.
	// Never dispose ReferenceCountedProviders with a lock because this may call into user code.
	oldRefCountedProviders.Dispose();

Changing the order would most likely be possible, but I don't see a possibility to do it in a simple way with low risk (I was searching for a fix that could be backported to 8.0)

[adamsitnik]

Share a single FIleProvider for default and don't dispose it. We still leak until finalization, but only one is leaked.

This is something I've not considered. How would it affect perf on various OSes? Is it better to have a single instance of PhysicalFilesWatcher rather than many?

runtime/src/libraries/Microsoft.Extensions.FileProviders.Physical/src/PhysicalFileProvider.cs

Line 32 in 1a7904e

privatePhysicalFilesWatcher?_fileWatcher;

Currently a new instance if being allocated if the user has not configured a default one (but I don't know whether for example ASP.NET does not do that somewhere else):

runtime/src/libraries/Microsoft.Extensions.Configuration.FileExtensions/src/FileConfigurationExtensions.cs

Line 44 in a971763

returnGetUserDefinedFileProvider(builder)??newPhysicalFileProvider(AppContext.BaseDirectory??string.Empty);

[adamsitnik]

Implement ref-counting

I am not a big fan of ref-counting (it's hardly ever implemented properly and always brings a lot of complexity).

or disposal pattern for the source. If we just add IDispose to the source impl, will it get called by whichever component manages it's lifetime today

From my perspective, the main issue is that ConfigurationRoot is given with a list of providers, but it does not have reference to the sources. So it can not dispose the sources with the current public API.

runtime/src/libraries/Microsoft.Extensions.Configuration/src/ConfigurationBuilder.cs

Lines 47 to 53 in a971763

	varproviders=newList<IConfigurationProvider>();
	foreach(IConfigurationSourcesourcein_sources)
	{
	IConfigurationProviderprovider=source.Build(this);
	providers.Add(provider);
	}
	returnnewConfigurationRoot(providers);

runtime/src/libraries/Microsoft.Extensions.Configuration/src/ConfigurationRoot.cs

Lines 99 to 112 in a971763

	publicvoidDispose()
	{
	// dispose change token registrations
	foreach(IDisposableregistrationin_changeTokenRegistrations)
	{
	registration.Dispose();
	}
	// dispose providers
	foreach(IConfigurationProviderproviderin_providers)
	{
	(providerasIDisposable)?.Dispose();
	}
	}

ConfigurationManager has both and can dispose either one or both of them.

In #100642 I've extended the provider abstraction with a reference to source (most of the implementations already implement a property that returns the source). It allows for disposing what is needed. But introducing a new public API would be a breaking change (so it can not be backported).

[ericstj]

Is this a safe assumption? Can't any FileConfigurationSource have Build called more than once?
...

ConfigurationManager is sealed, I could change this condition from to check for explicit type.

My point was that special casing ConfigurationManager isn't really a fix. Anyone can call

IConfigurationSourcesource=newDerivedFileConfigurationSource();using(IConfigurationProviderprovider=source.Build(builder)){}using(IConfigurationProviderprovider=source.Build(builder)){}

I believe it's intentional that Sources can produce multiple providers. So we really shouldn't assume that the provider can dispose resources held by the source.

Just create a new FileProvider if the previous one is disposed.
...
Changing the order would most likely be possible, but I don't see a possibility to do it in a simple way with low risk (I was searching for a fix that could be backported to 8.0)

Maybe you don't need to change the order - if your design is to delegate the lifetime of the FileProvider to the IConfigurationProvider then you should really be creating a new FileProvider for each call to Build. I think this is the solution that's most consistent with the change you made.

Share a single FIleProvider for default and don't dispose it. We still leak until finalization, but only one is leaked.
This is something I've not considered. How would it affect perf on various OSes? Is it better to have a single instance of PhysicalFilesWatcher rather than many?

I think it's a tradeoff (but @jozkee might have a better idea as he's our resident FSW expert). I think some systems have expense per-watcher like a background thread and kernel buffers - and those can approach a limit - so more of them uses more resources or could hit the limit - thus the whole polling watcher. From the same token those kernel buffers associated with a watcher limit how many events it can buffer between callbacks - so giving more work to a single one could result in events lost. At least for servicing - let's try to keep the same number of watchers as before. Folks can always set the provider if they want to experiment with a single FSW for perf. If you can see a better option for vNext we can consider that.

Agreed that RefCounting is too big and complex. I think a clear assignment of responsibility of lifetime for the file-provider is what we want. If it's possible to transfer the responsibility to the source and have the source's lifetime managed by something we could do that in the future. I don't think we should make the provider responsible for lifetime of resources held by the source - this regression made it clear that their lifetimes (and instance counts) differ.

[ericstj]

Bumping this @adamsitnik

[adamsitnik]

This PR fixes#95745 with low risk (to allow backporting), but in an ugly way as the existing public APIs don't allow for a proper fix and introducing a new API would require breaking changes.

[ericstj]

Is this a safe assumption? Can't any FileConfigurationSource have Build called more than once?

Also, I've noticed others implement IConfigurationBuilder before. Some are forks of ConfigurationManager, others are unique. I can share some data offline.

A couple other options to consider:

1. Just create a new FileProvider if the previous one is disposed. The logic for creating a new one isn't specific to the provider (it's implemented in an extension method we own IIRC).
2. Share a single FIleProvider for default and don't dispose it. We still leak until finalization, but only one is leaked.
3. Implement ref-counting, or disposal pattern for the source. If we just add IDispose to the source impl, will it get called by whichever component manages it's lifetime today?

[adamsitnik]

@ericstj Thank you for feedback! A lot of good questions! Here are my findings:

Is this a safe assumption? Can't any FileConfigurationSource have Build called more than once?

ConfigurationManager can call it more than once. It's called by ReloadSources which is called... anytime a source is being deleted, inserted or any property changes

Also, I've noticed others implement IConfigurationBuilder before. Some are forks of ConfigurationManager, others are unique.

ConfigurationManager is sealed, I could change this condition from to check for explicit type.

runtime/src/libraries/Microsoft.Extensions.Configuration/src/ConfigurationManager.cs

Line 25 in a971763

publicsealedclassConfigurationManager:IConfigurationManager,IConfigurationRoot,IDisposable

[adamsitnik]

Just create a new FileProvider if the previous one is disposed.

I was thinking about it. The idea I had was to null the FileProvider after disposing it and let ReloadSources re-assign it:

runtime/src/libraries/Microsoft.Extensions.Configuration/src/ConfigurationManager.cs

Line 146 in a971763

newProvidersList.Add(source.Build(this));

runtime/src/libraries/Microsoft.Extensions.Configuration.Json/src/JsonConfigurationSource.cs

Lines 18 to 20 in 1a7904e

	publicoverrideIConfigurationProviderBuild(IConfigurationBuilderbuilder)
	{
	EnsureDefaults(builder);

runtime/src/libraries/Microsoft.Extensions.Configuration.FileExtensions/src/FileConfigurationSource.cs

Line 71 in 1a7904e

FileProvider??=builder.GetFileProvider();

But then I've realized that ReloadSources creates new providers firsts, then tries to replace (it needs new ones to get rid of old ones) them:

runtime/src/libraries/Microsoft.Extensions.Configuration/src/ConfigurationManager.cs

Lines 136 to 157 in a971763

	privatevoidReloadSources()
	{
	DisposeRegistrations();
	_changeTokenRegistrations.Clear();
	varnewProvidersList=newList<IConfigurationProvider>();
	foreach(IConfigurationSourcesourcein_sources)
	{
	newProvidersList.Add(source.Build(this));
	}
	foreach(IConfigurationProviderpinnewProvidersList)
	{
	p.Load();
	_changeTokenRegistrations.Add(ChangeToken.OnChange(p.GetReloadToken,RaiseChanged));
	}
	_providerManager.ReplaceProviders(newProvidersList);
	RaiseChanged();
	}

and it tries to do that in thread-safe way:

runtime/src/libraries/Microsoft.Extensions.Configuration/src/ReferenceCountedProvidersManager.cs

Lines 41 to 58 in a971763

	publicvoidReplaceProviders(List<IConfigurationProvider>providers)
	{
	ReferenceCountedProvidersoldRefCountedProviders=_refCountedProviders;
	lock(_replaceProvidersLock)
	{
	if(_disposed)
	{
	thrownewObjectDisposedException(nameof(ConfigurationManager));
	}
	_refCountedProviders=ReferenceCountedProviders.Create(providers);
	}
	// Decrement the reference count to the old providers. If they are being concurrently read from
	// the actual disposal of the old providers will be delayed until the final reference is released.
	// Never dispose ReferenceCountedProviders with a lock because this may call into user code.
	oldRefCountedProviders.Dispose();

Changing the order would most likely be possible, but I don't see a possibility to do it in a simple way with low risk (I was searching for a fix that could be backported to 8.0)

[adamsitnik]

Share a single FIleProvider for default and don't dispose it. We still leak until finalization, but only one is leaked.

This is something I've not considered. How would it affect perf on various OSes? Is it better to have a single instance of PhysicalFilesWatcher rather than many?

runtime/src/libraries/Microsoft.Extensions.FileProviders.Physical/src/PhysicalFileProvider.cs

Line 32 in 1a7904e

privatePhysicalFilesWatcher?_fileWatcher;

Currently a new instance if being allocated if the user has not configured a default one (but I don't know whether for example ASP.NET does not do that somewhere else):

runtime/src/libraries/Microsoft.Extensions.Configuration.FileExtensions/src/FileConfigurationExtensions.cs

Line 44 in a971763

returnGetUserDefinedFileProvider(builder)??newPhysicalFileProvider(AppContext.BaseDirectory??string.Empty);

[adamsitnik]

Implement ref-counting

I am not a big fan of ref-counting (it's hardly ever implemented properly and always brings a lot of complexity).

or disposal pattern for the source. If we just add IDispose to the source impl, will it get called by whichever component manages it's lifetime today

From my perspective, the main issue is that ConfigurationRoot is given with a list of providers, but it does not have reference to the sources. So it can not dispose the sources with the current public API.

runtime/src/libraries/Microsoft.Extensions.Configuration/src/ConfigurationBuilder.cs

Lines 47 to 53 in a971763

	varproviders=newList<IConfigurationProvider>();
	foreach(IConfigurationSourcesourcein_sources)
	{
	IConfigurationProviderprovider=source.Build(this);
	providers.Add(provider);
	}
	returnnewConfigurationRoot(providers);

runtime/src/libraries/Microsoft.Extensions.Configuration/src/ConfigurationRoot.cs

Lines 99 to 112 in a971763

	publicvoidDispose()
	{
	// dispose change token registrations
	foreach(IDisposableregistrationin_changeTokenRegistrations)
	{
	registration.Dispose();
	}
	// dispose providers
	foreach(IConfigurationProviderproviderin_providers)
	{
	(providerasIDisposable)?.Dispose();
	}
	}

ConfigurationManager has both and can dispose either one or both of them.

In #100642 I've extended the provider abstraction with a reference to source (most of the implementations already implement a property that returns the source). It allows for disposing what is needed. But introducing a new public API would be a breaking change (so it can not be backported).

[ericstj]

Is this a safe assumption? Can't any FileConfigurationSource have Build called more than once?
...

ConfigurationManager is sealed, I could change this condition from to check for explicit type.

My point was that special casing ConfigurationManager isn't really a fix. Anyone can call

IConfigurationSourcesource=newDerivedFileConfigurationSource();using(IConfigurationProviderprovider=source.Build(builder)){}using(IConfigurationProviderprovider=source.Build(builder)){}

I believe it's intentional that Sources can produce multiple providers. So we really shouldn't assume that the provider can dispose resources held by the source.

Just create a new FileProvider if the previous one is disposed.
...
Changing the order would most likely be possible, but I don't see a possibility to do it in a simple way with low risk (I was searching for a fix that could be backported to 8.0)

Maybe you don't need to change the order - if your design is to delegate the lifetime of the FileProvider to the IConfigurationProvider then you should really be creating a new FileProvider for each call to Build. I think this is the solution that's most consistent with the change you made.

Share a single FIleProvider for default and don't dispose it. We still leak until finalization, but only one is leaked.
This is something I've not considered. How would it affect perf on various OSes? Is it better to have a single instance of PhysicalFilesWatcher rather than many?

I think it's a tradeoff (but @jozkee might have a better idea as he's our resident FSW expert). I think some systems have expense per-watcher like a background thread and kernel buffers - and those can approach a limit - so more of them uses more resources or could hit the limit - thus the whole polling watcher. From the same token those kernel buffers associated with a watcher limit how many events it can buffer between callbacks - so giving more work to a single one could result in events lost. At least for servicing - let's try to keep the same number of watchers as before. Folks can always set the provider if they want to experiment with a single FSW for perf. If you can see a better option for vNext we can consider that.

Agreed that RefCounting is too big and complex. I think a clear assignment of responsibility of lifetime for the file-provider is what we want. If it's possible to transfer the responsibility to the source and have the source's lifetime managed by something we could do that in the future. I don't think we should make the provider responsible for lifetime of resources held by the source - this regression made it clear that their lifetimes (and instance counts) differ.

[ericstj]

Bumping this @adamsitnik

[adamsitnik]

This PR fixes#95745 with low risk (to allow backporting), but in an ugly way as the existing public APIs don't allow for a proper fix and introducing a new API would require breaking changes.

[ericstj]

Is this a safe assumption? Can't any FileConfigurationSource have Build called more than once?

Also, I've noticed others implement IConfigurationBuilder before. Some are forks of ConfigurationManager, others are unique. I can share some data offline.

A couple other options to consider:

1. Just create a new FileProvider if the previous one is disposed. The logic for creating a new one isn't specific to the provider (it's implemented in an extension method we own IIRC).
2. Share a single FIleProvider for default and don't dispose it. We still leak until finalization, but only one is leaked.
3. Implement ref-counting, or disposal pattern for the source. If we just add IDispose to the source impl, will it get called by whichever component manages it's lifetime today?

[adamsitnik]

@ericstj Thank you for feedback! A lot of good questions! Here are my findings:

Is this a safe assumption? Can't any FileConfigurationSource have Build called more than once?

ConfigurationManager can call it more than once. It's called by ReloadSources which is called... anytime a source is being deleted, inserted or any property changes

Also, I've noticed others implement IConfigurationBuilder before. Some are forks of ConfigurationManager, others are unique.

ConfigurationManager is sealed, I could change this condition from to check for explicit type.

runtime/src/libraries/Microsoft.Extensions.Configuration/src/ConfigurationManager.cs

Line 25 in a971763

publicsealedclassConfigurationManager:IConfigurationManager,IConfigurationRoot,IDisposable

[adamsitnik]

Just create a new FileProvider if the previous one is disposed.

I was thinking about it. The idea I had was to null the FileProvider after disposing it and let ReloadSources re-assign it:

runtime/src/libraries/Microsoft.Extensions.Configuration/src/ConfigurationManager.cs

Line 146 in a971763

newProvidersList.Add(source.Build(this));

runtime/src/libraries/Microsoft.Extensions.Configuration.Json/src/JsonConfigurationSource.cs

Lines 18 to 20 in 1a7904e

	publicoverrideIConfigurationProviderBuild(IConfigurationBuilderbuilder)
	{
	EnsureDefaults(builder);

runtime/src/libraries/Microsoft.Extensions.Configuration.FileExtensions/src/FileConfigurationSource.cs

Line 71 in 1a7904e

FileProvider??=builder.GetFileProvider();

But then I've realized that ReloadSources creates new providers firsts, then tries to replace (it needs new ones to get rid of old ones) them:

runtime/src/libraries/Microsoft.Extensions.Configuration/src/ConfigurationManager.cs

Lines 136 to 157 in a971763

	privatevoidReloadSources()
	{
	DisposeRegistrations();
	_changeTokenRegistrations.Clear();
	varnewProvidersList=newList<IConfigurationProvider>();
	foreach(IConfigurationSourcesourcein_sources)
	{
	newProvidersList.Add(source.Build(this));
	}
	foreach(IConfigurationProviderpinnewProvidersList)
	{
	p.Load();
	_changeTokenRegistrations.Add(ChangeToken.OnChange(p.GetReloadToken,RaiseChanged));
	}
	_providerManager.ReplaceProviders(newProvidersList);
	RaiseChanged();
	}

and it tries to do that in thread-safe way:

runtime/src/libraries/Microsoft.Extensions.Configuration/src/ReferenceCountedProvidersManager.cs

Lines 41 to 58 in a971763

	publicvoidReplaceProviders(List<IConfigurationProvider>providers)
	{
	ReferenceCountedProvidersoldRefCountedProviders=_refCountedProviders;
	lock(_replaceProvidersLock)
	{
	if(_disposed)
	{
	thrownewObjectDisposedException(nameof(ConfigurationManager));
	}
	_refCountedProviders=ReferenceCountedProviders.Create(providers);
	}
	// Decrement the reference count to the old providers. If they are being concurrently read from
	// the actual disposal of the old providers will be delayed until the final reference is released.
	// Never dispose ReferenceCountedProviders with a lock because this may call into user code.
	oldRefCountedProviders.Dispose();

Changing the order would most likely be possible, but I don't see a possibility to do it in a simple way with low risk (I was searching for a fix that could be backported to 8.0)

[adamsitnik]

Share a single FIleProvider for default and don't dispose it. We still leak until finalization, but only one is leaked.

This is something I've not considered. How would it affect perf on various OSes? Is it better to have a single instance of PhysicalFilesWatcher rather than many?

runtime/src/libraries/Microsoft.Extensions.FileProviders.Physical/src/PhysicalFileProvider.cs

Line 32 in 1a7904e

privatePhysicalFilesWatcher?_fileWatcher;

Currently a new instance if being allocated if the user has not configured a default one (but I don't know whether for example ASP.NET does not do that somewhere else):

runtime/src/libraries/Microsoft.Extensions.Configuration.FileExtensions/src/FileConfigurationExtensions.cs

Line 44 in a971763

returnGetUserDefinedFileProvider(builder)??newPhysicalFileProvider(AppContext.BaseDirectory??string.Empty);

[adamsitnik]

Implement ref-counting

I am not a big fan of ref-counting (it's hardly ever implemented properly and always brings a lot of complexity).

or disposal pattern for the source. If we just add IDispose to the source impl, will it get called by whichever component manages it's lifetime today

From my perspective, the main issue is that ConfigurationRoot is given with a list of providers, but it does not have reference to the sources. So it can not dispose the sources with the current public API.

runtime/src/libraries/Microsoft.Extensions.Configuration/src/ConfigurationBuilder.cs

Lines 47 to 53 in a971763

	varproviders=newList<IConfigurationProvider>();
	foreach(IConfigurationSourcesourcein_sources)
	{
	IConfigurationProviderprovider=source.Build(this);
	providers.Add(provider);
	}
	returnnewConfigurationRoot(providers);

runtime/src/libraries/Microsoft.Extensions.Configuration/src/ConfigurationRoot.cs

Lines 99 to 112 in a971763

	publicvoidDispose()
	{
	// dispose change token registrations
	foreach(IDisposableregistrationin_changeTokenRegistrations)
	{
	registration.Dispose();
	}
	// dispose providers
	foreach(IConfigurationProviderproviderin_providers)
	{
	(providerasIDisposable)?.Dispose();
	}
	}

ConfigurationManager has both and can dispose either one or both of them.

In #100642 I've extended the provider abstraction with a reference to source (most of the implementations already implement a property that returns the source). It allows for disposing what is needed. But introducing a new public API would be a breaking change (so it can not be backported).

[ericstj]

Is this a safe assumption? Can't any FileConfigurationSource have Build called more than once?
...

ConfigurationManager is sealed, I could change this condition from to check for explicit type.

My point was that special casing ConfigurationManager isn't really a fix. Anyone can call

IConfigurationSourcesource=newDerivedFileConfigurationSource();using(IConfigurationProviderprovider=source.Build(builder)){}using(IConfigurationProviderprovider=source.Build(builder)){}

I believe it's intentional that Sources can produce multiple providers. So we really shouldn't assume that the provider can dispose resources held by the source.

Just create a new FileProvider if the previous one is disposed.
...
Changing the order would most likely be possible, but I don't see a possibility to do it in a simple way with low risk (I was searching for a fix that could be backported to 8.0)

Maybe you don't need to change the order - if your design is to delegate the lifetime of the FileProvider to the IConfigurationProvider then you should really be creating a new FileProvider for each call to Build. I think this is the solution that's most consistent with the change you made.

Share a single FIleProvider for default and don't dispose it. We still leak until finalization, but only one is leaked.
This is something I've not considered. How would it affect perf on various OSes? Is it better to have a single instance of PhysicalFilesWatcher rather than many?

I think it's a tradeoff (but @jozkee might have a better idea as he's our resident FSW expert). I think some systems have expense per-watcher like a background thread and kernel buffers - and those can approach a limit - so more of them uses more resources or could hit the limit - thus the whole polling watcher. From the same token those kernel buffers associated with a watcher limit how many events it can buffer between callbacks - so giving more work to a single one could result in events lost. At least for servicing - let's try to keep the same number of watchers as before. Folks can always set the provider if they want to experiment with a single FSW for perf. If you can see a better option for vNext we can consider that.

Agreed that RefCounting is too big and complex. I think a clear assignment of responsibility of lifetime for the file-provider is what we want. If it's possible to transfer the responsibility to the source and have the source's lifetime managed by something we could do that in the future. I don't think we should make the provider responsible for lifetime of resources held by the source - this regression made it clear that their lifetimes (and instance counts) differ.

[ericstj]

Bumping this @adamsitnik

[adamsitnik]

This PR fixes#95745 with low risk (to allow backporting), but in an ugly way as the existing public APIs don't allow for a proper fix and introducing a new API would require breaking changes.

[ericstj]

Is this a safe assumption? Can't any FileConfigurationSource have Build called more than once?

Also, I've noticed others implement IConfigurationBuilder before. Some are forks of ConfigurationManager, others are unique. I can share some data offline.

A couple other options to consider:

1. Just create a new FileProvider if the previous one is disposed. The logic for creating a new one isn't specific to the provider (it's implemented in an extension method we own IIRC).
2. Share a single FIleProvider for default and don't dispose it. We still leak until finalization, but only one is leaked.
3. Implement ref-counting, or disposal pattern for the source. If we just add IDispose to the source impl, will it get called by whichever component manages it's lifetime today?

[adamsitnik]

@ericstj Thank you for feedback! A lot of good questions! Here are my findings:

Is this a safe assumption? Can't any FileConfigurationSource have Build called more than once?

ConfigurationManager can call it more than once. It's called by ReloadSources which is called... anytime a source is being deleted, inserted or any property changes

Also, I've noticed others implement IConfigurationBuilder before. Some are forks of ConfigurationManager, others are unique.

ConfigurationManager is sealed, I could change this condition from to check for explicit type.

runtime/src/libraries/Microsoft.Extensions.Configuration/src/ConfigurationManager.cs

Line 25 in a971763

publicsealedclassConfigurationManager:IConfigurationManager,IConfigurationRoot,IDisposable

[adamsitnik]

Just create a new FileProvider if the previous one is disposed.

I was thinking about it. The idea I had was to null the FileProvider after disposing it and let ReloadSources re-assign it:

runtime/src/libraries/Microsoft.Extensions.Configuration/src/ConfigurationManager.cs

Line 146 in a971763

newProvidersList.Add(source.Build(this));

runtime/src/libraries/Microsoft.Extensions.Configuration.Json/src/JsonConfigurationSource.cs

Lines 18 to 20 in 1a7904e

	publicoverrideIConfigurationProviderBuild(IConfigurationBuilderbuilder)
	{
	EnsureDefaults(builder);

runtime/src/libraries/Microsoft.Extensions.Configuration.FileExtensions/src/FileConfigurationSource.cs

Line 71 in 1a7904e

FileProvider??=builder.GetFileProvider();

But then I've realized that ReloadSources creates new providers firsts, then tries to replace (it needs new ones to get rid of old ones) them:

runtime/src/libraries/Microsoft.Extensions.Configuration/src/ConfigurationManager.cs

Lines 136 to 157 in a971763

	privatevoidReloadSources()
	{
	DisposeRegistrations();
	_changeTokenRegistrations.Clear();
	varnewProvidersList=newList<IConfigurationProvider>();
	foreach(IConfigurationSourcesourcein_sources)
	{
	newProvidersList.Add(source.Build(this));
	}
	foreach(IConfigurationProviderpinnewProvidersList)
	{
	p.Load();
	_changeTokenRegistrations.Add(ChangeToken.OnChange(p.GetReloadToken,RaiseChanged));
	}
	_providerManager.ReplaceProviders(newProvidersList);
	RaiseChanged();
	}

and it tries to do that in thread-safe way:

runtime/src/libraries/Microsoft.Extensions.Configuration/src/ReferenceCountedProvidersManager.cs

Lines 41 to 58 in a971763

	publicvoidReplaceProviders(List<IConfigurationProvider>providers)
	{
	ReferenceCountedProvidersoldRefCountedProviders=_refCountedProviders;
	lock(_replaceProvidersLock)
	{
	if(_disposed)
	{
	thrownewObjectDisposedException(nameof(ConfigurationManager));
	}
	_refCountedProviders=ReferenceCountedProviders.Create(providers);
	}
	// Decrement the reference count to the old providers. If they are being concurrently read from
	// the actual disposal of the old providers will be delayed until the final reference is released.
	// Never dispose ReferenceCountedProviders with a lock because this may call into user code.
	oldRefCountedProviders.Dispose();

Changing the order would most likely be possible, but I don't see a possibility to do it in a simple way with low risk (I was searching for a fix that could be backported to 8.0)

[adamsitnik]

Share a single FIleProvider for default and don't dispose it. We still leak until finalization, but only one is leaked.

This is something I've not considered. How would it affect perf on various OSes? Is it better to have a single instance of PhysicalFilesWatcher rather than many?

runtime/src/libraries/Microsoft.Extensions.FileProviders.Physical/src/PhysicalFileProvider.cs

Line 32 in 1a7904e

privatePhysicalFilesWatcher?_fileWatcher;

Currently a new instance if being allocated if the user has not configured a default one (but I don't know whether for example ASP.NET does not do that somewhere else):

runtime/src/libraries/Microsoft.Extensions.Configuration.FileExtensions/src/FileConfigurationExtensions.cs

Line 44 in a971763

returnGetUserDefinedFileProvider(builder)??newPhysicalFileProvider(AppContext.BaseDirectory??string.Empty);

[adamsitnik]

Implement ref-counting

I am not a big fan of ref-counting (it's hardly ever implemented properly and always brings a lot of complexity).

or disposal pattern for the source. If we just add IDispose to the source impl, will it get called by whichever component manages it's lifetime today

From my perspective, the main issue is that ConfigurationRoot is given with a list of providers, but it does not have reference to the sources. So it can not dispose the sources with the current public API.

runtime/src/libraries/Microsoft.Extensions.Configuration/src/ConfigurationBuilder.cs

Lines 47 to 53 in a971763

	varproviders=newList<IConfigurationProvider>();
	foreach(IConfigurationSourcesourcein_sources)
	{
	IConfigurationProviderprovider=source.Build(this);
	providers.Add(provider);
	}
	returnnewConfigurationRoot(providers);

runtime/src/libraries/Microsoft.Extensions.Configuration/src/ConfigurationRoot.cs

Lines 99 to 112 in a971763

	publicvoidDispose()
	{
	// dispose change token registrations
	foreach(IDisposableregistrationin_changeTokenRegistrations)
	{
	registration.Dispose();
	}
	// dispose providers
	foreach(IConfigurationProviderproviderin_providers)
	{
	(providerasIDisposable)?.Dispose();
	}
	}

ConfigurationManager has both and can dispose either one or both of them.

In #100642 I've extended the provider abstraction with a reference to source (most of the implementations already implement a property that returns the source). It allows for disposing what is needed. But introducing a new public API would be a breaking change (so it can not be backported).

[ericstj]

Is this a safe assumption? Can't any FileConfigurationSource have Build called more than once?
...

ConfigurationManager is sealed, I could change this condition from to check for explicit type.

My point was that special casing ConfigurationManager isn't really a fix. Anyone can call

IConfigurationSourcesource=newDerivedFileConfigurationSource();using(IConfigurationProviderprovider=source.Build(builder)){}using(IConfigurationProviderprovider=source.Build(builder)){}

I believe it's intentional that Sources can produce multiple providers. So we really shouldn't assume that the provider can dispose resources held by the source.

Just create a new FileProvider if the previous one is disposed.
...
Changing the order would most likely be possible, but I don't see a possibility to do it in a simple way with low risk (I was searching for a fix that could be backported to 8.0)

Maybe you don't need to change the order - if your design is to delegate the lifetime of the FileProvider to the IConfigurationProvider then you should really be creating a new FileProvider for each call to Build. I think this is the solution that's most consistent with the change you made.

Share a single FIleProvider for default and don't dispose it. We still leak until finalization, but only one is leaked.
This is something I've not considered. How would it affect perf on various OSes? Is it better to have a single instance of PhysicalFilesWatcher rather than many?

I think it's a tradeoff (but @jozkee might have a better idea as he's our resident FSW expert). I think some systems have expense per-watcher like a background thread and kernel buffers - and those can approach a limit - so more of them uses more resources or could hit the limit - thus the whole polling watcher. From the same token those kernel buffers associated with a watcher limit how many events it can buffer between callbacks - so giving more work to a single one could result in events lost. At least for servicing - let's try to keep the same number of watchers as before. Folks can always set the provider if they want to experiment with a single FSW for perf. If you can see a better option for vNext we can consider that.

Agreed that RefCounting is too big and complex. I think a clear assignment of responsibility of lifetime for the file-provider is what we want. If it's possible to transfer the responsibility to the source and have the source's lifetime managed by something we could do that in the future. I don't think we should make the provider responsible for lifetime of resources held by the source - this regression made it clear that their lifetimes (and instance counts) differ.

[ericstj]

Bumping this @adamsitnik

[adamsitnik]

This PR fixes#95745 with low risk (to allow backporting), but in an ugly way as the existing public APIs don't allow for a proper fix and introducing a new API would require breaking changes.

[ericstj]

Is this a safe assumption? Can't any FileConfigurationSource have Build called more than once?

Also, I've noticed others implement IConfigurationBuilder before. Some are forks of ConfigurationManager, others are unique. I can share some data offline.

A couple other options to consider:

1. Just create a new FileProvider if the previous one is disposed. The logic for creating a new one isn't specific to the provider (it's implemented in an extension method we own IIRC).
2. Share a single FIleProvider for default and don't dispose it. We still leak until finalization, but only one is leaked.
3. Implement ref-counting, or disposal pattern for the source. If we just add IDispose to the source impl, will it get called by whichever component manages it's lifetime today?

[adamsitnik]

@ericstj Thank you for feedback! A lot of good questions! Here are my findings:

Is this a safe assumption? Can't any FileConfigurationSource have Build called more than once?

ConfigurationManager can call it more than once. It's called by ReloadSources which is called... anytime a source is being deleted, inserted or any property changes

Also, I've noticed others implement IConfigurationBuilder before. Some are forks of ConfigurationManager, others are unique.

ConfigurationManager is sealed, I could change this condition from to check for explicit type.

runtime/src/libraries/Microsoft.Extensions.Configuration/src/ConfigurationManager.cs

Line 25 in a971763

publicsealedclassConfigurationManager:IConfigurationManager,IConfigurationRoot,IDisposable

[adamsitnik]

Just create a new FileProvider if the previous one is disposed.

I was thinking about it. The idea I had was to null the FileProvider after disposing it and let ReloadSources re-assign it:

runtime/src/libraries/Microsoft.Extensions.Configuration/src/ConfigurationManager.cs

Line 146 in a971763

newProvidersList.Add(source.Build(this));

runtime/src/libraries/Microsoft.Extensions.Configuration.Json/src/JsonConfigurationSource.cs

Lines 18 to 20 in 1a7904e

	publicoverrideIConfigurationProviderBuild(IConfigurationBuilderbuilder)
	{
	EnsureDefaults(builder);

runtime/src/libraries/Microsoft.Extensions.Configuration.FileExtensions/src/FileConfigurationSource.cs

Line 71 in 1a7904e

FileProvider??=builder.GetFileProvider();

But then I've realized that ReloadSources creates new providers firsts, then tries to replace (it needs new ones to get rid of old ones) them:

runtime/src/libraries/Microsoft.Extensions.Configuration/src/ConfigurationManager.cs

Lines 136 to 157 in a971763

	privatevoidReloadSources()
	{
	DisposeRegistrations();
	_changeTokenRegistrations.Clear();
	varnewProvidersList=newList<IConfigurationProvider>();
	foreach(IConfigurationSourcesourcein_sources)
	{
	newProvidersList.Add(source.Build(this));
	}
	foreach(IConfigurationProviderpinnewProvidersList)
	{
	p.Load();
	_changeTokenRegistrations.Add(ChangeToken.OnChange(p.GetReloadToken,RaiseChanged));
	}
	_providerManager.ReplaceProviders(newProvidersList);
	RaiseChanged();
	}

and it tries to do that in thread-safe way:

runtime/src/libraries/Microsoft.Extensions.Configuration/src/ReferenceCountedProvidersManager.cs

Lines 41 to 58 in a971763

	publicvoidReplaceProviders(List<IConfigurationProvider>providers)
	{
	ReferenceCountedProvidersoldRefCountedProviders=_refCountedProviders;
	lock(_replaceProvidersLock)
	{
	if(_disposed)
	{
	thrownewObjectDisposedException(nameof(ConfigurationManager));
	}
	_refCountedProviders=ReferenceCountedProviders.Create(providers);
	}
	// Decrement the reference count to the old providers. If they are being concurrently read from
	// the actual disposal of the old providers will be delayed until the final reference is released.
	// Never dispose ReferenceCountedProviders with a lock because this may call into user code.
	oldRefCountedProviders.Dispose();

Changing the order would most likely be possible, but I don't see a possibility to do it in a simple way with low risk (I was searching for a fix that could be backported to 8.0)

[adamsitnik]

Share a single FIleProvider for default and don't dispose it. We still leak until finalization, but only one is leaked.

This is something I've not considered. How would it affect perf on various OSes? Is it better to have a single instance of PhysicalFilesWatcher rather than many?

runtime/src/libraries/Microsoft.Extensions.FileProviders.Physical/src/PhysicalFileProvider.cs

Line 32 in 1a7904e

privatePhysicalFilesWatcher?_fileWatcher;

Currently a new instance if being allocated if the user has not configured a default one (but I don't know whether for example ASP.NET does not do that somewhere else):

runtime/src/libraries/Microsoft.Extensions.Configuration.FileExtensions/src/FileConfigurationExtensions.cs

Line 44 in a971763

returnGetUserDefinedFileProvider(builder)??newPhysicalFileProvider(AppContext.BaseDirectory??string.Empty);

[adamsitnik]

Implement ref-counting

I am not a big fan of ref-counting (it's hardly ever implemented properly and always brings a lot of complexity).

or disposal pattern for the source. If we just add IDispose to the source impl, will it get called by whichever component manages it's lifetime today

From my perspective, the main issue is that ConfigurationRoot is given with a list of providers, but it does not have reference to the sources. So it can not dispose the sources with the current public API.

runtime/src/libraries/Microsoft.Extensions.Configuration/src/ConfigurationBuilder.cs

Lines 47 to 53 in a971763

	varproviders=newList<IConfigurationProvider>();
	foreach(IConfigurationSourcesourcein_sources)
	{
	IConfigurationProviderprovider=source.Build(this);
	providers.Add(provider);
	}
	returnnewConfigurationRoot(providers);

runtime/src/libraries/Microsoft.Extensions.Configuration/src/ConfigurationRoot.cs

Lines 99 to 112 in a971763

	publicvoidDispose()
	{
	// dispose change token registrations
	foreach(IDisposableregistrationin_changeTokenRegistrations)
	{
	registration.Dispose();
	}
	// dispose providers
	foreach(IConfigurationProviderproviderin_providers)
	{
	(providerasIDisposable)?.Dispose();
	}
	}

ConfigurationManager has both and can dispose either one or both of them.

In #100642 I've extended the provider abstraction with a reference to source (most of the implementations already implement a property that returns the source). It allows for disposing what is needed. But introducing a new public API would be a breaking change (so it can not be backported).

[ericstj]

Is this a safe assumption? Can't any FileConfigurationSource have Build called more than once?
...

ConfigurationManager is sealed, I could change this condition from to check for explicit type.

My point was that special casing ConfigurationManager isn't really a fix. Anyone can call

IConfigurationSourcesource=newDerivedFileConfigurationSource();using(IConfigurationProviderprovider=source.Build(builder)){}using(IConfigurationProviderprovider=source.Build(builder)){}

I believe it's intentional that Sources can produce multiple providers. So we really shouldn't assume that the provider can dispose resources held by the source.

Just create a new FileProvider if the previous one is disposed.
...
Changing the order would most likely be possible, but I don't see a possibility to do it in a simple way with low risk (I was searching for a fix that could be backported to 8.0)

Maybe you don't need to change the order - if your design is to delegate the lifetime of the FileProvider to the IConfigurationProvider then you should really be creating a new FileProvider for each call to Build. I think this is the solution that's most consistent with the change you made.

Share a single FIleProvider for default and don't dispose it. We still leak until finalization, but only one is leaked.
This is something I've not considered. How would it affect perf on various OSes? Is it better to have a single instance of PhysicalFilesWatcher rather than many?

I think it's a tradeoff (but @jozkee might have a better idea as he's our resident FSW expert). I think some systems have expense per-watcher like a background thread and kernel buffers - and those can approach a limit - so more of them uses more resources or could hit the limit - thus the whole polling watcher. From the same token those kernel buffers associated with a watcher limit how many events it can buffer between callbacks - so giving more work to a single one could result in events lost. At least for servicing - let's try to keep the same number of watchers as before. Folks can always set the provider if they want to experiment with a single FSW for perf. If you can see a better option for vNext we can consider that.

Agreed that RefCounting is too big and complex. I think a clear assignment of responsibility of lifetime for the file-provider is what we want. If it's possible to transfer the responsibility to the source and have the source's lifetime managed by something we could do that in the future. I don't think we should make the provider responsible for lifetime of resources held by the source - this regression made it clear that their lifetimes (and instance counts) differ.

[ericstj]

Bumping this @adamsitnik