simpleinjector / SimpleInjector

An easy, flexible, and fast Dependency Injection library that promotes best practice to steer developers towards the pit of success.
https://simpleinjector.org
MIT License
1.21k stars 154 forks source link

Question: GetInstanceForConsumer? (Class Interception and conditional registrations) #958

Closed GeraldLx closed 2 years ago

GeraldLx commented 2 years ago

I am trying to get some performance logging into a badly designed legacy application. Sadly it works with hundreds of class registrations (Actually it is even worse, I have to set ResolveUnregisteredTypes to true, but that does not matter for the problem).

In a very simplified way, we have something like this:

public interface ILogger
{
  void Log(string message);
}

public interface ILogger<T> : ILogger
{
}

public sealed class ConsoleLogger<T> : ILogger<T>
{
  void ILogger.Log(string message)
  {
    var type = typeof(T);

    Console.WriteLine($"{type}: {message}");
  }
}

public class Foo
{
  private readonly ILogger _logger;
  public Foo(ILogger logger)
  {
    _logger = logger;
  }

  public virtual void Bar()
  {
    _logger.Log(nameof(Bar));
  }
}

Of course the real interceptor will look different, but a simple one can look like this:

public sealed class MonitoringInterceptor : IInterceptor
{
  void IInterceptor.Intercept(IInvocation invocation)
  {
    Console.WriteLine($"Before {invocation.Method.Name}");

    invocation.Proceed();

    Console.WriteLine($"After {invocation.Method.Name}");
  }
}

I basically have already in place a variant of what you are describing under AOP on the Simple Injector page using Castle Dynamic Proxy behind the scenes. It works fantastic for interface registrations, but as written, the old code uses classes direct.

Initially I thought all I have to do is use CreateClassProxyWithTarget instead of CreateInterfaceProxyWithTarget. This works as long as the classes do not have constructors with parameters.

There is an overload with the constructorArguments, so I am currently trying to use that one. But how do I get the arguments? I am trying to use GetInstance, but that does interfere with conditional registrations, as the consumer is not set.

Also simplified, that is a demo code of what I am trying to do:

public static class Program
{
  private static readonly ProxyGenerator Generator = new();
  private static readonly MonitoringInterceptor Interceptor = new();

  public static void Main()
  {
    var container = new Container();
    container.Register<ITest, Test>();
    container.Register<Foo>();
    container.RegisterConditional(
        typeof(ILogger),
        c => typeof(ConsoleLogger<>).MakeGenericType(c.Consumer!.ImplementationType),
        Lifestyle.Singleton,
        c => c.HasConsumer);

    container.ExpressionBuilt += OnContainerExpressionBuilt;

    container.Verify();

    var foo = container.GetInstance<Foo>();
    foo.Bar();
  }

  private static void OnContainerExpressionBuilt(object? sender, ExpressionBuiltEventArgs e)
  {
    var container = (Container)sender!;
    var serviceType = e.RegisteredServiceType;

    var options = new ProxyGenerationOptions();

    Func<Type, object, ProxyGenerationOptions, IInterceptor, object> createProxy;

    if (serviceType.IsInterface)
    {
      createProxy = (p, t, o, i) => Generator.CreateInterfaceProxyWithTarget(p, t, o, i);
    }
    else if (serviceType.IsClass && !serviceType.IsSealed)
    {
      var behavior = container.Options.ConstructorResolutionBehavior;

      createProxy = (p, t, o, i) =>
      {
        var arguments = behavior.GetConstructor(p)
          .GetParameters()
          .Select(p => container.GetInstance(p.ParameterType))
          .ToArray();

        return Generator.CreateClassProxyWithTarget(p, t, o, arguments, i);
      };
    }
    else
    {
      return;
    }

    e.Expression = Expression.Convert(
        Expression.Invoke(
          Expression.Constant(createProxy),
          Expression.Constant(serviceType, typeof(Type)),
          e.Expression,
          Expression.Constant(options),
          Expression.Constant(Interceptor)),
        serviceType);
  }
}

Of course this version crashes, as the container cannot resolve ILogger. So basically I want to get an instance of ILogger for the consumer serviceType.

Is there a way to achieve this or am I completely wrong and should use a different approach for class interception?

dotnetjunkie commented 2 years ago

The call to CreateClassProxyWithTarget takes on the responsibility of creating the type. This is typically something you would like Simple Injector to do, because in that case, Simple Injector is able to inject an ConsoleLogger<FooProxy> into the FooProxy class.

This, however, is quite tricky to achieve, because Castle wants to create the object itself. Furthermore, the proxy type generated contains constructor arguments for the target (i.e. decoratee) and the interceptors, making it even harder to instruct Simple Injector to construct such type.

As there exists no GetInstanceForConsumer method in Simple Injector, there are two workarounds I can think of:

CORRECTION: There actually is such feature. Please read the next answer to see how to do this. The remaining text is left here for educational purposes. Still, the text below contains a warning that might still be relevant.

One is to simply inject null arguments into the proxy class. This works in case the proxy was able to override all methods of the base class, as in that case it only uses the decoratee, which was created by Simple Injector anyway. For instance:

else if (serviceType.IsClass && !serviceType.IsSealed)
{
    var behavior = container.Options.ConstructorResolutionBehavior;

    // Create a fixed list with null values. Will be reused.
    object[] arguments =
        behavior.GetConstructor(serviceType).GetParameters().Select(p1 => (object)null).ToArray();

    createProxy = (classToProxy, target, o, i) =>
    {
        return Generator.CreateClassProxyWithTarget(classToProxy, target, o, arguments, i);
    };
}

If, however, the intercepted class might contain non-virtual methods, it means that a consumer will in fact call methods on the Castle-created instance, not the decorated 'target'. This means you need a different method.

WARNING: From the last paragraph above it becomes clear that there will be 2 Foo instances created (one Foo by Simple Injector and one FooProxy by Castle). You should be very sure that this doesn't cause any problems, because it can easily lead to Torn Lifestyle issues.

So in this case, probably the easiest workaround is to allow the conditional registration to return a type in case the registration is resolved directly; as happens in your case. For instance:

container.RegisterConditional(
    typeof(Other.ILogger),
    c => typeof(Other.ConsoleLogger<>)
        .MakeGenericType(c.Consumer?.ImplementationType ?? typeof(object)),
    Lifestyle.Singleton,
    c => true);

Using this registration Foo will still be injected with a ConsoleLogger<Foo>, while FooProxy is injected with a ConsoleLogger<object>. Whether this is a problem I cannot tell.

But to be honest, I'm afraid that you are getting yourself into an utter mess if you try to continue this path. Trying to apply interception on your concrete types is fragile and it will likely complicate your code beyond comprehension by other developers. If possible, I'd suggest slowly and surely to make small refactorings such that those concrete types are hidden behind abstractions. In most code bases, hiding those types behind abstractions is a relatively easy and safe refactoring.

Either way: good luck.

dotnetjunkie commented 2 years ago

Correction, I seem to be forgetting how my own code base works. There does actually exists an "GetInstanceForConsumer" method. In short, this is how to do it:

container.Options.DependencyInjectionBehavior
    .GetInstanceProducer(new InjectionConsumerInfo(...), throwOnFailure: true)
    .GetInstance();

Within the context of your code, it would be something like the following:

if (serviceType.IsInterface)
{
    createProxy = (p, t, o, i) => Generator.CreateInterfaceProxyWithTarget(p, t, o, i);
}
else if (serviceType.IsClass && !serviceType.IsSealed)
{
    var behavior = container.Options.ConstructorResolutionBehavior;
    var injector = container.Options.DependencyInjectionBehavior;

    var producers =
        behavior.GetConstructor(serviceType).GetParameters()
        .Select(p => injector.GetInstanceProducer(new InjectionConsumerInfo(p), true))
        .ToArray();

    createProxy = (p, t, o, i) =>
    {
        var arguments = producers.Select(producer => producer.GetInstance()).ToArray();

        return Generator.CreateClassProxyWithTarget(p, t, o, arguments, i);
    };
}
else
{
    return;
}

The meat of the operation is in the call to Container.Options.DependencyInjectionBehavior.GetInstanceProducer. This GetInstanceProducer method is supplied with an InjectionConsumerInfo which describes the information for the type to be resolved and its consumer (which is all contained in the ParameterInfo instance), and it produces an InstanceProducer. This instance producer can than, later on, be used to produce a new instance, which is what you see in the code example, where the InstanceProducer instances are obtained before the lambda, while their instances are requested inside the lambda.

I hope this helps.

GeraldLx commented 2 years ago

Thank you for being persistent, your solution works even better than the one before and does not have the disadvantages I described before.

I experimented with it last week, works like a charm. Of course the old legacy code crashes here and then with interception in place, cause they use calls like GetType().xxx.

But these problems cannot be fixed by SimpleInjector.

Thanks for your support