dotnet / aspnetcore

ASP.NET Core is a cross-platform .NET framework for building modern cloud-based web applications on Windows, Mac, or Linux.
https://asp.net
MIT License
35.37k stars 9.99k forks source link

AddOption/Service.Configure creating duplicated entries #38569

Closed jhonathanalmeida closed 2 years ago

jhonathanalmeida commented 2 years ago

Describe the bug

When injecting settings on WebApplication Builder, 2 instances of the same injections are made: builder.Services.Configure<MySettings>(builder.Configuration.GetSection("MySettings")); OR builder.Services.AddOptions<MySettings>().Bind(builder.Configuration.GetSection("MySettings"));

This only happens when using the WebApplication Builder, but works fine when using the old Generic Host through Program.Main + Startup file.

P.S. Found this behavior when migrating a new project from .Net5 to .Net6.

To Reproduce

Issue can be reproduced by using the following solution: https://github.com/jhonathanalmeida/DotNet6_AddOption_Test

There are two projects, one testing with GRPC and other with REST/Web.

In both projects you can see that there is only one injection of the settings, such as: builder.Services.AddOptions<MySettings>().Bind(builder.Configuration.GetSection("MySettings"));

And then, once the application is running, you can call either the "WeatherForecastController" or "GreeterService", and during the constructor call you will see that it will log an error as it has more then one item in the list, and then throw the "InvalidOperationException" with the message " Sequence contains more than one element".

Exceptions (if any)

The exception occurs when calling the IEnumerable<IOptions<MySettings>> mySettings.Single().Value.

System.InvalidOperationException
  HResult=0x80131509
  Message=Sequence contains more than one element
  Source=System.Linq
  StackTrace:
   at System.Linq.ThrowHelper.ThrowMoreThanOneElementException()
   at System.Linq.Enumerable.TryGetSingle[TSource](IEnumerable`1 source, Boolean& found)
   at System.Linq.Enumerable.Single[TSource](IEnumerable`1 source)
   at WebAPICore.Controllers.WeatherForecastController..ctor(ILogger`1 logger, IEnumerable`1 mySettings) in C:\Users\jhonathan.almeida\source\repos\DotNet6_AddOption_Test\WebAPICore\Controllers\WeatherForecastController.cs:line 30
   at Microsoft.AspNetCore.Mvc.Controllers.ControllerActivatorProvider.<>c__DisplayClass7_0.<CreateActivator>b__0(ControllerContext controllerContext)
   at Microsoft.AspNetCore.Mvc.Controllers.ControllerFactoryProvider.<>c__DisplayClass6_0.<CreateControllerFactory>g__CreateController|0(ControllerContext controllerContext)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.Next(State& next, Scope& scope, Object& state, Boolean& isCompleted)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.InvokeInnerFilterAsync()

Further technical details

.NET SDK (reflecting any global.json): Version: 6.0.100 Commit: 9e8b04bbff

Runtime Environment: OS Name: Windows OS Version: 10.0.19043 OS Platform: Windows RID: win10-x64 Base Path: C:\Program Files\dotnet\sdk\6.0.100\

Host (useful for support): Version: 6.0.0 Commit: 4822e3c3aa

.NET SDKs installed: 5.0.402 [C:\Program Files\dotnet\sdk] 6.0.100 [C:\Program Files\dotnet\sdk]

.NET runtimes installed: Microsoft.AspNetCore.All 2.1.30 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.All] Microsoft.AspNetCore.App 2.1.30 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App] Microsoft.AspNetCore.App 3.1.20 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App] Microsoft.AspNetCore.App 5.0.11 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App] Microsoft.AspNetCore.App 5.0.12 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App] Microsoft.AspNetCore.App 6.0.0 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App] Microsoft.NETCore.App 2.1.30 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App] Microsoft.NETCore.App 3.1.20 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App] Microsoft.NETCore.App 5.0.11 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App] Microsoft.NETCore.App 5.0.12 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App] Microsoft.NETCore.App 6.0.0 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App] Microsoft.WindowsDesktop.App 3.1.20 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App] Microsoft.WindowsDesktop.App 5.0.11 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App] Microsoft.WindowsDesktop.App 5.0.12 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App] Microsoft.WindowsDesktop.App 6.0.0 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]

ghost commented 2 years ago

Thanks for contacting us. We're moving this issue to the .NET 7 Planning milestone for future evaluation / consideration. Because it's not immediately obvious that this is a bug in our framework, we would like to keep this around to collect more feedback, which can later help us determine the impact of it. We will re-evaluate this issue, during our next planning meeting(s). If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues. To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

halter73 commented 2 years ago

This is the result of how WebApplicationBuilder currently shares service descriptors with the underlying HostBuilder. I recently opened https://github.com/dotnet/runtime/issues/61635 to improve this integration in .NET 7. I think we'll want to share the IServiceCollection more directly going forward.

AddOptions wouldn't normally ever add service descriptors more than once, but WebApplicationBuilder effectively calls AddOptions on two different IServiceCollections and merges these collections together.

I'm a little surprised you found this @jhonathanalmeida. Why are the apps trying to resolve IEnumerable<IOptions<MySettings>> to begin with instead of just Options<MySettings>? We could consider patching to remove the duplicate service descriptors. Is there code in the wild that relies on there only being exactly one IOptions<T> service?

Stacks where AddOptions is called ``` Microsoft.Extensions.Options.dll!Microsoft.Extensions.DependencyInjection.OptionsServiceCollectionExtensions.AddOptions(Microsoft.Extensions.DependencyInjection.IServiceCollection services) Unknown Microsoft.Extensions.Logging.dll!Microsoft.Extensions.DependencyInjection.LoggingServiceCollectionExtensions.AddLogging(Microsoft.Extensions.DependencyInjection.IServiceCollection services, System.Action configure) Unknown Microsoft.Extensions.Hosting.dll!Microsoft.Extensions.Hosting.HostingHostBuilderExtensions.ConfigureLogging.AnonymousMethod__0(Microsoft.Extensions.Hosting.HostBuilderContext context, Microsoft.Extensions.DependencyInjection.IServiceCollection collection) Unknown Microsoft.AspNetCore.dll!Microsoft.AspNetCore.Hosting.BootstrapHostBuilder.RunDefaultCallbacks(Microsoft.Extensions.Configuration.ConfigurationManager configuration, Microsoft.Extensions.Hosting.HostBuilder innerBuilder) Unknown Microsoft.AspNetCore.dll!Microsoft.AspNetCore.Builder.WebApplicationBuilder.WebApplicationBuilder(Microsoft.AspNetCore.Builder.WebApplicationOptions options, System.Action configureDefaults) Unknown Microsoft.AspNetCore.dll!Microsoft.AspNetCore.Builder.WebApplication.CreateBuilder(string[] args) Unknown WebAPICore.dll!Program.
$(string[] args) Line 3 C# ``` ``` Microsoft.Extensions.Options.dll!Microsoft.Extensions.DependencyInjection.OptionsServiceCollectionExtensions.AddOptions(Microsoft.Extensions.DependencyInjection.IServiceCollection services) Unknown Microsoft.Extensions.Hosting.dll!Microsoft.Extensions.Hosting.HostBuilder.CreateServiceProvider() Unknown Microsoft.Extensions.Hosting.dll!Microsoft.Extensions.Hosting.HostBuilder.Build() Unknown Microsoft.AspNetCore.dll!Microsoft.AspNetCore.Builder.WebApplicationBuilder.Build() Unknown WebAPICore.dll!Program.
$(string[] args) Line 21 C# ```
jhonathanalmeida commented 2 years ago

Hey @halter73, thanks for sharing that info!

Our real life scenario is actually a bit more complex.

We are developing a shared library for our internal developers, and a few of the implementations we have for instance, for Azure KeyVault, or Azure ServiceBus, we rely on injecting the configuration using AddOptions.

The thing is, one of the teams needed to actually access two different instances of the service bus, in the same application, so we tried to use named injections, which .Net 5 DI does not allow right? So what we did is, we created Identifiable Configurations. This is done by having a IIdentifiableConfiguration interface, like:

public interface IIdentifiableConfiguration
{
    string ConfigurationName { get; set; }
}

And then each injection will have this value set to it's respective "ConfigurationName ", so each service/instance can retrieve their own version of the configuration.

To exemplify, let's say we have a DBSettings class, and I need multiple injections of if, because my App need to connect to two different DBs.

So DBSettings implements this interface, and we get:

public class DBSettings : IIdentifiableConfiguration
{
    public string ConfigurationName { get; set; } = string.Empty;
    public string ConnStr { get; set; } = string.Empty;
}

And then, we would have something like this in the AppSettings:

{
"DBSettings": {
    "DB_A": {
      "ConfigurationName": "DBAKey",
      "ConnStr": "DB A Conn Str"
    },
    "DB_B": {
      "ConfigurationName": "DBBKey",
      "ConnStr": "DB B Conn Str"
    }
  } 

And then we would have these injections:

builder.Services.AddOptions<DBSettings>()
    .Bind(builder.Configuration.GetSection("DBSettings:DB_A"));
builder.Services.AddOptions<DBSettings>()
    .Bind(builder.Configuration.GetSection("DBSettings:DB_B"));

And finally in the Service we would have:

public class GreeterServiceA
{
    private const string DBConnKey = "DBAKey";

    private readonly DBSettings _dbSettings;

    public GreeterServiceA(IEnumerable<IOptions<DBSettings>> allDBSettings)
    {
        _dbSettings= allDBSettings.Select(sc => sc.Value)
            .Single(sc => sc.ConfigurationName == DBConnKey );
    }

    .
    .
    .
}

This way, each service can identify it's own instance of the configuration.

I know it is a bit messy, I'll create a realish example, and push it to the Repo above.

jhonathanalmeida commented 2 years ago

Ok, just pushed another example containing a somewhat similar approach to what we are doing. You can find the code in this project (under the solution/repo above).

However I'd like to highlight two points. 1- Named option injections exists (I really missed it out on our initial research), and I was able to make it work fine (commented out lines in program.cs + _namedOptionsAcessor in GreeterServiceA). 2- When trying to inject two configurations based on the same object, only the last instance prevails, and everything else is discarded. That can be tested by calling the GreeterServiceA, it will throw an exception stating that there is no matching element for "ServiceAKey".

Just for reference, this pattern of option injection we were using was working fine on .Net 5.

Also, just to give you some more info, in our real implementation, there are also two instances of the same object injected, for example a DBClient, so in order to identify them, each one is injected with a identification as well, and this is done pretty similarly to what we did with the Configurations, by implementing a IIdentifiableObject interface, which has a Key, which is then injected in the instance by a factory patter.

We just found out that named option Injections exists, but I don't think named injections exists, do they? So I can inject multiple instances of the same class/service?

Thanks once again for all the help/information.

Best regards

Jhonny

halter73 commented 2 years ago

However I'd like to highlight two points. 1- Named option injections exists (I really missed it out on our initial research), and I was able to make it work fine (commented out lines in program.cs + _namedOptionsAcessor in GreeterServiceA).

Glad to hear this! Named options do seem like the right approach here.

2- When trying to inject two configurations based on the same object, only the last instance prevails, and everything else is discarded. That can be tested by calling the GreeterServiceA, it will throw an exception stating that there is no matching element for "ServiceAKey".

This is by design. The same thing should happen in .NET 5.

We just found out that named option Injections exists, but I don't think named injections exists, do they? So I can inject multiple instances of the same class/service?

Correct. Named services aren't a concept that's built in to Microsoft.Extensions.DependencyInjection. We recommend using the factory pattern if you want named services similar to what we do with IOptionsFactory<TOptions>.

Just for reference, this pattern of option injection we were using was working fine on .Net 5.

Do you have an example project demonstrating the pattern you're trying to use working in .NET 5? I think that would better help me understand the scenario.

I don't think calling AddOptions multiple times should have ever added more than one IOptions<TOptions> service in .NET 5 meaning IEnumerable<IOptions<DBSettings>> allDBSettings should have never had more than one element unless the app was adding an IOptions<DBSettings> service using something other than AddOptions or it was merging IServiceCollections in a similar way to WebApplicationBuilder.

ghost commented 2 years ago

This issue has been resolved and has not had any activity for 1 day. It will be closed for housekeeping purposes.

See our Issue Management Policies for more information.