Disposable Transient Component diagnostic message is a bit misleading

[AroglDarthu]

With the transition from V4.x to V5.x, we found an unfortunate error in our diagnostic suppression logic for certain disposable transients. So kudos for adding auto verification, as it lead to the discovery of a mistake. It took me a bit to find out why the logic was OK in V4.x but not in V5.x.

TL;DR

1. Disposable Transient Component Suppression ultimately falls back on the implementation type. However, the violation warning should hint at the contract type(s) used for registration! Why? Because that contract type is needed for calling GetRegistration (which is needed for adding the suppression).
2. Calling GetRegistration(type, bool) seems to add a new transient registration (V4.x) for concrete types. This now fails in V5.x, because it disallows resolving unregistered concrete types. Shouldn't GetRegistration just return null instead?
3. Side-question: the API documentation on GetRegistration states that the overloads without the throwOnFailure flag lock the container, whereas the ones with the flag might lock the container. Is that (still) correct?

Short repro

static class Program
{
    static void Main(string[] args)
    {
        var container = new Container();
        container.Register<IFoo, Qux>(Lifestyle.Transient);
        container.Register<IBar, Qux>(Lifestyle.Transient);

        try
        {
            // The following line generates an exception
            //var registration0 = container.GetRegistration(typeof(Qux), true)!.Registration;
            //registration0.SuppressDiagnosticWarning(DiagnosticType.DisposableTransientComponent, "Justification");

            // These lines do not. Only one of these registrations is really necessary, but it works with both as well.
            var registration1 = container.GetRegistration(typeof(IFoo), true)!.Registration;
            registration1.SuppressDiagnosticWarning(DiagnosticType.DisposableTransientComponent, "Justification");
            var registration2 = container.GetRegistration(typeof(IBar), true)!.Registration;
            registration2.SuppressDiagnosticWarning(DiagnosticType.DisposableTransientComponent, "Justification");
        }
        catch (Exception ex)
        {
            Console.WriteLine(ex.Message);
            Console.WriteLine(ex.ToString());
        }

        Console.WriteLine("Press key to exit");
        Console.ReadLine();
    }
}

public interface IFoo : IDisposable
{
}

public interface IBar
{
}

public class Qux : IFoo, IBar
{
    public void Dispose()
    {
    }
}

Output

The configuration is invalid. The following diagnostic warnings were reported: -[Disposable Transient Component] Qux is registered as transient, but implements IDisposable. See the Error property for detailed information about the warnings. Please see https://simpleinjector.org/diagnostics how to fix problems and how to suppress individual warnings. Verification was triggered because Container.Options.EnableAutoVerification was enabled. To prevent the container from being verified on first resolve, set Container.Options.EnableAutoVerification to false. SimpleInjector.ActivationException: The configuration is invalid. The following diagnostic warnings were reported: -[Disposable Transient Component] Qux is registered as transient, but implements IDisposable. See the Error property for detailed information about the warnings. Please see https://simpleinjector.org/diagnostics how to fix problems and how to suppress individual warnings. Verification was triggered because Container.Options.EnableAutoVerification was enabled. To prevent the container from being verified on first resolve, set Container.Options.EnableAutoVerification to false. ---> SimpleInjector.DiagnosticVerificationException: The configuration is invalid. The following diagnostic warnings were reported: -[Disposable Transient Component] Qux is registered as transient, but implements IDisposable. See the Error property for detailed information about the warnings. Please see https://simpleinjector.org/diagnostics how to fix problems and how to suppress individual warnings. at SimpleInjector.Container.ThrowOnDiagnosticWarnings() at SimpleInjector.Container.NotifyAndLock() --- End of inner exception stack trace --- at SimpleInjector.Container.NotifyAndLock() at SimpleInjector.Container.GetRegistration(Type serviceType, Boolean throwOnFailure) at SimpleInjector5DisposableTransientsSuppression.Program.Main(String[] args) in C:\Sandbox\SimpleInjector5DisposableTransientsSuppression\SimpleInjector5DisposableTransientsSuppression\Program.cs:line 35 Press key to exit

Elaborate Use Case

We are using some boilerplate code that helps to register WCF clients. The auto generated code for Service References contains multiple constructors, and is therefore not very handy for consumption in Simple Injector. To reproduce the problem, I just copied our boilerplate code to a console app. Unfortunately that resulted in a different error than the one from our code base. It took me a while to realize that the error was thrown by System.ServiceModel.ClientBase<TChannel>:

The configuration is invalid. Creating the instance for type WcfClient<FooServiceClient, IFooService> failed. Attempted to get contract type for IFooService, but that type is not a ServiceContract, nor does it inherit a ServiceContract. Verification was triggered because Container.Options.EnableAutoVerification was enabled. To prevent the container from being verified on first resolve, set Container.Options.EnableAutoVerification to false.

At first it all looked completely Simple Injector related to me. Closer inspection revealed that it was not:

---> System.InvalidOperationException: Attempted to get contract type for IFooService, but that type is not a ServiceContract, nor does it inherit a ServiceContract.    at System.ServiceModel.Description.ServiceReflector.GetContractTypeAndAttribute(Type interfaceType, ServiceContractAttribute& contractAttribute)

So let's get that out of the way by removing the inheritance on System.ServiceModel.ClientBase<TChannel> alltogether. Now the hint is very clear:

The configuration is invalid. The following diagnostic warnings were reported: -[Disposable Transient Component] WcfClient<FooServiceClient, IFooService> is registered as transient, but implements Disposable. -[Disposable Transient Component] WcfClient<BarServiceClient, IBarService> is registered as transient, but implements Disposable. -[Disposable Transient Component] BarServiceClient is registered as transient, but implements IDisposable.

Apparently the first suppression worked, as there is no warning about FooServiceClient. That leads to the conclusion that the second suppression failed and triggered an automatic container verification.

The code for registering our Wcf Clients contains two transient registrations for each Client type. It looks like this:

public static void RegisterWcfClientFactory<TChannel, TClient>(Container container)
    where TChannel : class
    where TClient : ClientBase<TChannel>
{
    Func<IWcfClient<TChannel>> creator = container.GetInstance<IWcfClient<TChannel>>;

    container.Register<TClient>(Lifestyle.Transient);
    container.Register<IWcfClient<TChannel>, WcfClient<TClient, TChannel>>(Lifestyle.Transient);
    container.RegisterInstance(creator);
    container.Register<IWcfClientFactory<TChannel>, WcfClientFactory<TChannel>>(Lifestyle.Singleton);
}

Our suppression logic basically does the following:

foreach (var serviceType in disposableComponents)
{
    var registration = container.GetRegistration(serviceType, true)!.Registration;
    registration.SuppressDiagnosticWarning(DiagnosticType.DisposableTransientComponent, Justification);
}

Now, where does this all go wrong? The culprit lies in the types we lookup. As the Simple Injector message states that the implementation type is registered as transient, we build a list of types containing the implementation types. So in this case the list contains:

• typeof(FooServiceClient)
• typeof(WcfClient<FooServiceClient, IFooService>)
• typeof(BarServiceClient)
• typeof(WcfClient<BarServiceClient, IBarService>)

In V4.x this works. In V5.x it does not. My guess would be that the call to GetRegistration(typeof(WcfClient<...>)) considers the given service type as not registered? V4.x silently adds a new transient registration, but V5.x does not? Why doen't it just return null instead? I'm not trying to resolve anything and I certainly don't want the container starting to verify at this point in my bootstrapping logic.

The solution for us is just to add the suppression for IWcfClient<...> instead, as that is the one we registered in the first place. Easy fix.

[dotnetjunkie]

TLDR;

• GetRegistation is too strict in its locking behavior and this should be changed. I created #863 for this.
• The diagnostic message can be improved. I created #864 for this.
• Calling GetRegistration(Type, true) currently never returns null. Changing this is a breaking change.
• The behavior you saw with v4 (i.e. with ResolveUnregisteredConcreteTypes = true) is caused by Simple Injector's Torn-Lifestyle prevention feature that was introduced in v4. A more detailed explanation of how to combat the change with ResolveUnregisteredConcreteTypes in v5, please read this.

Thank you for this elaborate description. There are many things going on here, but let me first start with your last question: why does this work on v4, and not in v5? Let's take a look at your first registrations to explain this:

container.Register<IFoo, Qux>(Lifestyle.Transient);
container.Register<IBar, Qux>(Lifestyle.Transient);

Here two seperate registrations are made for the same implementation type Qux. For Simple Injector, this means:

• Both registrations get the same Registration instance. A Registration is responsible for producing instances of a single implementation type (read: Qux) for a given lifestyle. Or to be more exact: a Registration produces the expression tree.
• Each registration, however, gets it's own InstanceProducer instance. An InstanceProducer wraps a Registration and can apply interception (read: decorators) to the registration based on the service type (read: IFoo and IBar).

So in respect to the previous registrations, the following assertions hold:

// note that this part of the API is a bit weird, unfortunetely, because GetRegistration returns InstanceProducer.
InstanceProducer f = container.GetRegistration(typeof(IFoo));
InstanceProducer b = container.GetRegistration(typeof(IBar));

Assert.AreNotSame(f, b);
Assert.AreSame(f.Registration, b.Registration);

The fact that Simple Injector will reuse the same Registration object is the reason why your code worked in v4. In v4, due to the fact that Container.Options.ResolveUnregisteredConcreteTypes was true by default, a call to GetRegistration for a concrete unregistered type, would result in an implicit registration for that type. In other words, this call:

InstanceProducer q = container.GetRegistration(typeof(Qux));

would implicitly result in this registration:

container.Register<Qux, Qux>(Lifestyle.Transient);

And that new InstanceProducer would wrap the same Registration as the registrations for both IFoo and IBar. And because diagnostic warnings are suppressed on the Registration, it doesn't matter which InstanceProducer you pick. In other words, the following assertions hold when ResolveUnregisteredConcreteTypes is true:

container.Register<IFoo, Qux>(Lifestyle.Transient);

InstanceProducer f = container.GetRegistration(typeof(IFoo));
InstanceProducer q = container.GetRegistration(typeof(Qux));

Assert.AreNotSame(f, q);
Assert.AreSame(f.Registration, q.Registration);

What changed in v5 is that ResolveUnregisteredConcreteTypes is now false. This will make GetRegistration(typeof(Qux)) return null and GetRegistration(typeof(Qux), throwOnFailure: true) throw an exception.

Of course, what happened in v4 was that a new, accidental, and unused registration (read: InstanceProducer) was added. It didn't do much harm, but I'd argue that not adding an unused registration is cleaner.

Shouldn't GetRegistration just return null instead?

The current design is for GetRegistration to never return null when throwOnFailure is true. It assumes a not-found registration to be a failure. This seem reasonable to me, but eitherways, changing that contract has quite some impact. It's quite a severe breaking change.

Another question is whether it is correct for GetRegistration to lock, even when it returns null. It's currently quite aggressive when it comes to locking. This means that it currently almost always locks. This aggressive behavious exists from quite some time (possibly v3, and was loosened up with #529 in v4.1.1), but the triggering of the verification in v5 can trigger the exception quite early.

Reason for its aggressive locking behavior is because many things can happen under the cover when calling GetRegistration, even when null is returned. Especially the triggering of the ResolveUnregisteredType even is the reason of this aggressive locking. However, I think this locking behavior can—and should—be loosened up, because I can't think of a scenario where GetRegistration should directly lock the container. GetRegistration would cause implicit registrations, but they are simply that: registrations. There are still some cases where GetRegistration could trigger a lock, which is when you call it on List<T> or an array, because that would call into GetAllInstances, which would cause a lock, but those are corner cases. I'll create a issue #863 for this.

With respect to the clarity of the diagnostic warning, you are right and this can be approved. I created issue #864 for this.

I hope this answers your questions.

[AroglDarthu]

Thanks for the clarification. Certainly answers the questions and I'm glad we got the gist of it right.

About GetRegistration in combination with throwOnFailure == true... You're right, of course, and it also says as much in the API documentation. I just read it a little different and somehow didn't classify not having the requested registration as a failure ;-)

Are there any other circumstances where the method could throw, or is not having a registration the only one?

[dotnetjunkie]

Are there any other circumstances where the method could throw, or is not having a registration the only one?

GetRegistration(Type, throwOnFailure: true) throws an exception when:

1. The supplied Type is a null reference
2. The supplied Type contains generic parameters
3. The container has already been disposed
4. Multiple applicable registrations found for the requested Type. This can happen with overlapping conditional registrations.
5. There is a verification error (as GetRegistration locks the container, and locking triggers verification)
6. When Type is a DependencyMetadata<T> where T is a type for what point 4. holds.
7. A hooked unregistered-type resolution event throws an exception on the supplied Type.
8. In case an unregistered concrete type is resolved while ResolveUnregisteredConcreteTypes = true, that can't be created (e.g. because it has multiple ctors, or has unregistered dependencies).

[AroglDarthu]

Ok, I can see why returning null is a breaking change for the API. Might I suggest a feature then? container.HasRegistrationFor(Type serviceType)

[dotnetjunkie]

But wouldn't that behavior be equal to container.GetRegistration(type) != null?

[AroglDarthu]

You would think so, but there is a difference. It's not the same when considering all the paths where an exception could also lead to it (GetRegistration) returning null.

HasRegistrationFor would only return false if the requested Registration is, uhm, legitimate (for lack of a better word), but just not registered. I certainly wouldn't mind HasRegistrationFor throwing an exception if I feed it a bogus type; bogus in equals bogus out.

Mmm, doesn't seem very usefull... maybe just forget I asked. It's friday night and there is beer in play ;-)

[dotnetjunkie]

It's not the same when considering all the paths where an exception could also lead to it (GetRegistration) returning null.

Exceptions thrown from underneath would typically not cause GetRegistration to return null.

HasRegistrationFor would only return false if the requested Registration is, uhm, legitimate (for lack of a better word), but just not registered.

Ah, I see what you mean. GetRegistration can trip unregistered type resolution, which would allow the registration to be added and returned, without you having called Register<X, T>() explicitly. In that case, the following code would yield the HasRegistrationFor behavior:

container.GetCurrentRegistrations().Any(p => p.ServiceType == typeof(IMyServiceType))

GetCurrentRegistrations just returns what's currently there. The returned list might very very different after container.Verify() is called, because of unregistered type resolution.

[AroglDarthu]

Cool. Didn't think of that one ;-) Also, GetCurrentRegistrations won't ever lock the container. Nice.

[dotnetjunkie]

There's one possible downside... It calls ToArray everytime you call it. So prevent it from being called hundreds or thousands of times.

[AroglDarthu]

Does feel a bit weird though, accessing all registrations when you know exactly which one you're looking for. However, for us the problem has been solved with correctly registering the service type. So we'll just use GetRegistration and it won't blow up anymore because we have no unregistered types.

In other scenarios, where it is actually unknown if a registration has been made, GetCurrentRegistrations can be used. It seems unlikely that it needs to be called too often. We actually have a situation where we use it. A package of ours provides a bootstrapper. The package can be used directly in an application, or it can be used through other packages. The bootstrapper internally calls GetCurrentRegistrations to see if it needs to do anything.