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.

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

Convey is disposing IConfiguration object and breaks reloadable IConfigurationProviders supplied to application #91

Closed
shoter opened this issue Oct 16, 2021 · 3 comments

Comments

@shoter
Copy link

shoter commented Oct 16, 2021

Repository with reproducible example

https://github.com/shoter/Convey-Configuration-Error

How to reproduce the problem

  1. Create configuration provider (e.x TimeProvider in this linked repository) that reloads IConfiguration when configuration source changes.
  2. Use that provider through ConfigureAppConfiguration
  3. Add Convey to application in ConfigureServices. services.AddConvey() is enough to trigger the error.

After executing those steps all configuration providers are going to stop reporting configuration changes.

Workaround

You can create simple service class that requires IConfiguration which is going to be reloaded when you manually detect a moment when it should be reloaded.

Example code:

var configuration = configuration as ConfigurationRoot;

// (...)

configuration.Reload();

Where the issue originates

Issue resides inside Extensions.cs:GetOptions. Every time when Convey wants to get options from service collection it builds new service provider based on service collection from main application and retrieves IConfiguration to fetch options values.
By using word using on ServiceProvider Convey ensures it is going to be disposed along with objects it created. This is going to dispose IConfiguration object which is ConfigurationRoot under the hood. ConfigurationRoot is going to dispose all configuration providers attached to it which is going to break reloadable functionality.

Proposed solutions

  1. Stop building service provider from service collection provided by users. It can have unintended side effects that are very hard to diagnose.

Instead supply your convey builder with IConfiguration instance in addition to ServiceCollection. IConfiguration should be easily accessible from every Startup.cs. Therefore all existing users should be able to modify their code with ease to cope with this change.
Unfortunately it is going to be a breaking change :/.

Additionally this approach would require an inner instance of ServiceCollection to instantiate your objects during configuring process. All configured objects could be added to main application's ServiceCollection that was provided to Convey. (Personally I think this should be reworked as it looks weird to constantly build and dispose service providers. This is my own opinion though)

This is an approach that is going to eliminate all possible side effects of using ServiceCollection you do not own.

  1. Supply ConveyBuilder with IConfiguration and rewrite method Extensions.cs:GetOptions to use it. There might be other methods that needs to be rewritten to use this approach
@shoter
Copy link
Author

shoter commented Dec 6, 2021

@spetz @GooRiOn

Any update on this?
For me this is a bug that is blocking me from introducing Convey to my projects. It is just going to do haywire in at least Configuration objects.

@spetz
Copy link
Member

spetz commented Dec 15, 2021

Apologies for the delayed response - I will take a look into this, e.g. if the IConfiguration could be passed to the ConveyBuilder and used internally within the other components' extension methods. Otherwise, I'd need to introduce some breaking changes, which, hopefully, could be avoided.

@spetz
Copy link
Member

spetz commented Dec 28, 2021

Fixed in the latest release - when calling AddConvey() an optional IConfiguration instance can be passed - if it's null, then the default behaviour (building with IServiceProvider) will be used.

@spetz spetz closed this as completed Dec 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants