LoggerFactory constructor overloads break external service providers

[nphmuller]

Commit 355b186 introduced additional constructors for the LoggerFactory class. The class can still be resolved by the default service provider, but external providers (I use Autofac) get confused and don't know which constructor to use.

The workaround is to specify during configuration of the service provider which constructor to use, but that's of course not as nice as when things simply work. Especially since LoggerFactory is used in basically every ASP.NET Core application, every user of Autofac (and possibly other 3rd party service providers) would have to configure LoggerFactory themselves.

I run into this problem during the migration from version 1.x to 2.0 and it took me a while to figure out what the problem was. Maybe I've missed it, but it has not been announced as a breaking change either. (Maybe it doesn't count as breaking?)

The reason I create this issue here and not at Autofac's repo, is that other 3rd party service providers are likely to run into the same problem.

[davidfowl]

Hmm, that's bad. We'll need to figure out how to fix this in 2.1.

/cc @pakrym

[MaxDeg]

Just for the log. Here is a work around that worked for us.

services.AddSingleton<ILoggerFactory>(sp => new LoggerFactory(sp.GetServices<ILoggerProvider>()));

[nphmuller]

@MaxDeg
Woops, forgot to add the workaround. The default service provider uses a different overload:

services.AddSingleton<ILoggerFactory, LoggerFactory>(sp =>
    new LoggerFactory(
        sp.GetRequiredService<IEnumerable<ILoggerProvider>>(),
        sp.GetRequiredService<IOptionsMonitor<LoggerFilterOptions>>()
    )
);

[pakrym]

@nphmuller do you have a repro sample?

I tried:

1. dotnet new web
2. project.csproj

<Project Sdk="Microsoft.NET.Sdk.Web">

  <PropertyGroup>
    <TargetFramework>netcoreapp2.1</TargetFramework>
  </PropertyGroup>

  <ItemGroup>
    <Folder Include="wwwroot\" />
  </ItemGroup>

  <ItemGroup>
    <PackageReference Include="Microsoft.AspNetCore.All" Version="2.0.0" />
    <PackageReference Include="Autofac.Extensions.DependencyInjection" Version=" 4.2.0" />
  </ItemGroup>

</Project>

3. Program.cs

using System;
using System.Collections.Generic;
using System.Linq;
using System.Threading.Tasks;
using Microsoft.AspNetCore.Builder;
using Microsoft.AspNetCore.Hosting;
using Microsoft.AspNetCore.Http;
using Microsoft.Extensions.DependencyInjection;
using Autofac;
using Autofac.Extensions.DependencyInjection;

namespace _21
{
    public class Startup
    {
        // This method gets called by the runtime. Use this method to add services to the container.
        // For more information on how to configure your application, visit https://go.microsoft.com/fwlink/?LinkID=398940
        public void ConfigureServices(IServiceCollection services)
        {
        }

        public void ConfigureContainer(ContainerBuilder builder)
        {
        }

        // This method gets called by the runtime. Use this method to configure the HTTP request pipeline.
        public void Configure(IApplicationBuilder app, IHostingEnvironment env)
        {
            if (env.IsDevelopment())
            {
                app.UseDeveloperExceptionPage();
            }

            app.Run(async (context) =>
            {
                await context.Response.WriteAsync("Hello World!");
            });
        }
    }
}

And things seem to be working fine.

[nphmuller]

@pakrym I tried to repro the issue and you're right, it's not as straightforward or as severe as I thought. Not even sure if it warrants a fix.

Here's a repro repo: https://github.com/nphmuller/AuotfacLogDI
It differs in only one way from your sample:

public void ConfigureContainer(ContainerBuilder builder)
{
    builder.RegisterSource(
        new AnyConcreteTypeNotAlreadyRegisteredSource()
            .WithRegistrationsAs(b => b.InstancePerLifetimeScope()));
}

As you can see, we configure Autofac to resolve all types not explicitly configured with a Scoped lifetime.
Because of this Autofac suddenly recognizes both of these constructor overloads of LoggerFactory as valid:

// Also valid when AnyConcreteTypeNotAlreadyRegisteredSource is used.
public LoggerFactory(IEnumerable<ILoggerProvider> providers, LoggerFilterOptions filterOptions);

// Valid before AnyConcreteTypeNotAlreadyRegisteredSource was used.
public LoggerFactory(IEnumerable<ILoggerProvider> providers, IOptionsMonitor<LoggerFilterOptions> filterOption);

I know it's not good practice to make every type resolvable by default, and we plan to refactor our code so we don't need it any more. But honestly, we never had this problem before. Again: not sure if it warrants a fix though.

[pakrym]

The fix would be to add another single constructor with more arguments so it get preferred over those two, but I don't think we are willing to take it at this point.

Can you exclude types from AnyConcreteTypeNotAlreadyRegisteredSource? Excluding all asp.net core assemblies should fix this problem and prevent future bugs.

[davidfowl]

Agreed. I'm not sure we want this warrants a fix.

[nphmuller]

@pakrym Yeah it's pretty easy to exclude types. AnyConcretTypeNotAlreadyRegisteredSource's constructor overload takes a Func<Type, bool> predicate.

I'll think about making a PR for Autofac to exclude at least this type, or maybe all aspnetcore types. Think this issue fits better in their project then this one now anyway.

[pakrym]

Closing this.