Expanded captive dependency check

[hvanbakel]

Captive dependency checking checked for singletons being dependent upon a scoped dependency. However, depending on a dependency which lifetime is shorter than its parent is not specific to that relation. This PR adds checks for Singleton -> Transient and Scoped -> Transient.

[hvanbakel]

So this expands the check from just Scoped dependencies to a lifetime check. I'm open to making this a completely separate check based on a separate boolean in configuration as well.

[khellang]

Even though this highlights potential bugs/leaks in your code, and the scope checking isn't enabled by default outside the Development environment, this is a breaking change. I wonder why @pakrym didn't add these checks in the first place?

[hvanbakel]

@khellang correct, so maybe adding this as a separate option is a better idea as that would make it non-breaking

[khellang]

Looks like it might've just been overlooked/forgotten... #430

[khellang]

I guess it could be kept under the same flag, but using something similar to MVC's CompatibilitySwitch. Adding flags to methods gets really messy quickly.

[pakrym]

Capturing transitive from singletons/scopes is often completely valid scenario, this would cause too many false positives.

[hvanbakel]

@pakrym how is that the case? That is exactly the behavior that you're trying to prevent by setting the ValidateScopes flag to true?

A transitive in a singleton becomes a singleton.

[pakrym]

Often transitive is not more then "I want a new instance every time because I have internal state and don't care about how long my object would be alive". Also, this change would make transitive useless in ASP.NET Core, because you are always in request scope.

[hvanbakel]

So if the abstraction is that you get a new instance every single time, how does that hold if you're resolving a transient dependency inside a singleton?

Are you saying that every dependency in my controller has request scope?

[pakrym]

It still holds you resolved one singleton instance you got one dependency instance.

If in the controller you call services.GetService<SomeService>() and SomeService happens to be IDisposible is would be captured and stored in the service provider scope and would not be disposed until scope is disposed.

[hvanbakel]

Ok so let's assume that as the implementor of an interface, I do not know who takes a dependency on me (as it should) and I would like to use internal state. How do I guarantee that one of my consumers does not make me a singleton scoped instance?

Take this below code for example, when I'm developing ITransientDependency I am making the assumption that I will be instantiated every single time. Isn't this exactly what the developer of for example IOptionsSnapshot assumes?

public class TestController : Controller
    {
        private readonly ISingletonDependency dependency;

        public TestController(
            ISingletonDependency dependency)
        {
            this.dependency = dependency;
        }

        [HttpGet]
        [Route("")]
        public int AreEqual()
        {
            return this.dependency.Dependency.Count;
        }
    }

    public interface ISingletonDependency
    {
        ITransientDependency Dependency { get; }
    }

    public class SingletonDependency : ISingletonDependency
    {
        public SingletonDependency(ITransientDependency dependency)
        {
            this.Dependency = dependency;
        }

        public ITransientDependency Dependency { get; private set; }
    }

    public interface ITransientDependency
    {
        int Count { get; }
    }

    public class TransientDependency : ITransientDependency
    {
        private int count = 0;

        public int Count => ++this.count;
    }

[pakrym]

How do I guarantee that one of my consumers does not make me a singleton scoped instance?

Why would you care about this?

[hvanbakel]

Let me turn the question around, why do you care about a singleton taking a dependency on a scoped dependency?
You care because it breaks what you were trying to do. Take a look at IOptionsSnapshot<T> for example, that is an example of where the current check throws as it is trying to read on every request and wrapping that in a singleton makes it break.

[pakrym]

Because we've seen bugs caused by it and it wasn't aggressively breaking a lot of code. Also transient does not denote how "long" the services would live just that it "Transient lifetime services are created each time they're requested. "

There was a discussion about capturing IDisposible instances in root container: #456

Did you try running MVC app with this checks enabled?

[hvanbakel]

I did, it doesn't fail and it increases the count with every request. Here's my program.cs:

    public class Program
    {
        public static void Main(string[] args)
        {
            BuildWebHost(args).Run();
        }

        public static IWebHost BuildWebHost(string[] args) =>
            WebHost.CreateDefaultBuilder(args)
                .UseDefaultServiceProvider((context, options) =>
                {
                    options.ValidateScopes = context.HostingEnvironment.IsDevelopment();
                })
                .UseStartup<Startup>()
                .Build();
    }

and startup

    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)
        {
            services.AddMvc();

            services.AddTransient<ITransientDependency, TransientDependency>();
            services.AddScoped<IScopedDependency, ScopedDependency>();
            services.AddSingleton<ISingletonDependency, SingletonDependency>();
        }

        // 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.UseMvc();
        }
    }

[pakrym]

Strange. There are a lot of MVC singleton services that cache transients:

https://github.com/aspnet/Mvc/blob/1d6b02c1f56ef46bc4c8568e748747c82bbf1bb8/src/Microsoft.AspNetCore.Mvc.Core/DependencyInjection/MvcCoreServiceCollectionExtensions.cs#L187-L190

[hvanbakel]

So the issue here is that nesting a transient in a singleton does not fail.

So singleton > scoped > transient (in lifetime)
So nesting a scoped in a singleton expands the lifetime of the scoped dependency and crashes
Nesting a transient in a singleton also expands the lifetime but doesn't crash
The line above is what this PR addresses

[pakrym]

@hvanbakel Did you try running MVC with your additional check enabled?

[hvanbakel]

I did, so now I get:

+		$exception	{System.InvalidOperationException: Cannot consume transient service 'Microsoft.Extensions.Options.IOptionsFactory`1[Microsoft.AspNetCore.Server.Kestrel.Core.KestrelServerOptions]' from singleton 'Microsoft.Extensions.Options.IOptions`1[Microsoft.AspNetCore.Server.Kestrel.Core.KestrelServerOptions]'.
   at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteValidator.VisitTransient(TransientCallSite transientCallSite, CallSiteValidatorState state)
   at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteVisitor`2.VisitCallSite(IServiceCallSite callSite, TArgument argument)
   at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteValidator.VisitConstructor(ConstructorCallSite constructorCallSite, CallSiteValidatorState state)
   at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteVisitor`2.VisitCallSite(IServiceCallSite callSite, TArgument argument)
   at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteValidator.VisitSingleton(SingletonCallSite singletonCallSite, CallSiteValidatorState state)
   at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteVisitor`2.VisitCallSite(IServiceCallSite callSite, TArgument argument)
   at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteValidator.VisitConstructor(ConstructorCallSite constructorCallSite, CallSiteValidatorState state)
   at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteVisitor`2.VisitCallSite(IServiceCallSite callSite, TArgument argument)
   at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteValidator.VisitSingleton(SingletonCallSite singletonCallSite, CallSiteValidatorState state)
   at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteVisitor`2.VisitCallSite(IServiceCallSite callSite, TArgument argument)
   at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteValidator.ValidateCallSite(IServiceCallSite callSite)
   at Microsoft.Extensions.DependencyInjection.ServiceProvider.Microsoft.Extensions.DependencyInjection.ServiceLookup.IServiceProviderEngineCallback.OnCreate(IServiceCallSite callSite)
   at Microsoft.Extensions.DependencyInjection.ServiceLookup.ServiceProviderEngine.CreateServiceAccessor(Type serviceType)
   at System.Collections.Concurrent.ConcurrentDictionary`2.GetOrAdd(TKey key, Func`2 valueFactory)
   at Microsoft.Extensions.DependencyInjection.ServiceLookup.ServiceProviderEngine.GetService(Type serviceType, ServiceProviderEngineScope serviceProviderEngineScope)
   at Microsoft.Extensions.DependencyInjection.ServiceLookup.ServiceProviderEngine.GetService(Type serviceType)
   at Microsoft.Extensions.DependencyInjection.ServiceProvider.GetService(Type serviceType)
   at Microsoft.Extensions.DependencyInjection.ServiceProviderServiceExtensions.GetRequiredService(IServiceProvider provider, Type serviceType)
   at Microsoft.Extensions.DependencyInjection.ServiceProviderServiceExtensions.GetRequiredService[T](IServiceProvider provider)
   at Microsoft.AspNetCore.Hosting.Internal.WebHost.EnsureServer()
   at Microsoft.AspNetCore.Hosting.Internal.WebHost.BuildApplication()}	System.InvalidOperationException

which is good. Because this signals that Microsoft.Extensions.Options.IOptionsFactory now has singleton lifetime inside of Microsoft.Extensions.Options.IOptions.

It's a bit odd to crash for singleton wrapping a scoped but not for this though as it is essentially the same problem of 1 component with a long lifetime wrapping a component with a short lifetime.