autofac / Autofac

An addictive .NET IoC container
https://autofac.org
MIT License
4.47k stars 836 forks source link

OnActivated() not working together with EnableInterfaceInterceptors() #860

Closed bcmedeiros closed 3 years ago

bcmedeiros commented 7 years ago

I was struggling with some weird InvalidCastException nested inside a Autofac.Core.DependencyResolutionException in my application and after a few hours I could narrow the problem down to the following code:

using System;
using Autofac;
using Autofac.Extras.DynamicProxy;
using Castle.DynamicProxy;

namespace AutofacTest
{

    public interface IA
    {
        string Hello();
    }
    public class A : IA
    {
        public string Hello()
        {
            return "hello world!";
        }
    }

    public class DummyInterceptor : IInterceptor
    {
        public void Intercept(IInvocation invocation)
        {
            Console.Write("before ");
            invocation.Proceed();
        }
    }

    public class Program
    {

        static void Main(string[] args)
        {
            // Create your builder.
            var builder = new ContainerBuilder();

            builder.Register(a => new DummyInterceptor());

            var r = builder.RegisterType<A>()
                .As<IA>()
                .OnActivated(a => Console.WriteLine( a.GetType().ToString() ))
                .EnableInterfaceInterceptors()
                .InterceptedBy(typeof(DummyInterceptor));

            var c = builder.Build();

            var ia = c.Resolve<IA>();

            Console.WriteLine(ia.Hello());
            Console.ReadKey();

        }
    }

}
Autofac.Core.DependencyResolutionException occurred
  HResult=0x80131500
  Message=An exception was thrown while executing a resolve operation. See the InnerException for details.
  Source=Autofac
  StackTrace:
   at Autofac.Core.Resolving.ResolveOperation.Execute(IComponentRegistration registration, IEnumerable`1 parameters)
   at Autofac.Core.Lifetime.LifetimeScope.ResolveComponent(IComponentRegistration registration, IEnumerable`1 parameters)
   at Autofac.Core.Container.ResolveComponent(IComponentRegistration registration, IEnumerable`1 parameters)
   at Autofac.ResolutionExtensions.TryResolveService(IComponentContext context, Service service, IEnumerable`1 parameters, Object& instance)
   at Autofac.ResolutionExtensions.ResolveService(IComponentContext context, Service service, IEnumerable`1 parameters)
   at Autofac.ResolutionExtensions.Resolve[TService](IComponentContext context, IEnumerable`1 parameters)
   at Autofac.ResolutionExtensions.Resolve[TService](IComponentContext context)
   at AutofacTest.Program.Main(String[] args) in ....\Projects\ConsoleApp2\ConsoleApp2\Program.cs:line 47

Inner Exception 1:
InvalidCastException: Unable to cast object of type 'Castle.Proxies.IAProxy' to type 'A'.

In the original code, I got a little bit deeper in Autofac code:

>   Autofac.dll!Autofac.Builder.RegistrationBuilder<CCC.Scheduling.BusinessLayer.Services.StockpileBusinessService, Autofac.Builder.ConcreteReflectionActivatorData, Autofac.Builder.SingleRegistrationStyle>.OnActivated.AnonymousMethod__0(object s, Autofac.Core.ActivatedEventArgs<object> e)   Unknown
    Autofac.dll!Autofac.Core.Resolving.InstanceLookup.Complete()    Unknown
    Autofac.dll!Autofac.Core.Resolving.ResolveOperation.CompleteActivations()   Unknown
    Autofac.dll!Autofac.Core.Resolving.ResolveOperation.GetOrCreateInstance(Autofac.Core.ISharingLifetimeScope currentOperationScope, Autofac.Core.IComponentRegistration registration, System.Collections.Generic.IEnumerable<Autofac.Core.Parameter> parameters)  Unknown
    Autofac.dll!Autofac.Core.Resolving.ResolveOperation.Execute(Autofac.Core.IComponentRegistration registration, System.Collections.Generic.IEnumerable<Autofac.Core.Parameter> parameters)    Unknown
    Autofac.dll!Autofac.ResolutionExtensions.TryResolveService(Autofac.IComponentContext context, Autofac.Core.Service service, System.Collections.Generic.IEnumerable<Autofac.Core.Parameter> parameters, out object instance) Unknown
    Autofac.dll!Autofac.ResolutionExtensions.ResolveService(Autofac.IComponentContext context, Autofac.Core.Service service, System.Collections.Generic.IEnumerable<Autofac.Core.Parameter> parameters) Unknown
    Autofac.dll!Autofac.ResolutionExtensions.Resolve<CCC.Scheduling.BusinessLayer.Interface.IStockpileBusinessService>(Autofac.IComponentContext context, System.Collections.Generic.IEnumerable<Autofac.Core.Parameter> parameters)    Unknown

This problem seems to be related to the https://github.com/autofac/Autofac/blob/develop/src/Autofac/Builder/RegistrationBuilder%7BTLimit%2CTActivatorData%2CTRegistrationStyle%7D.cs class, in the onActivated() method, the handler is called with a cast, like (TLimit)e.Instance), which seems unlikely to work with a interface proxy.

Using:

<?xml version="1.0" encoding="utf-8"?>
<packages>
  <package id="Autofac" version="4.6.0" targetFramework="net452" />
  <package id="Autofac.Extras.DynamicProxy" version="4.2.1" targetFramework="net452" />
  <package id="Castle.Core" version="4.0.0" targetFramework="net452" />
</packages>

I'm reluctant to believe I just found a bug in a very likely feature combination like this, but I couldn't find any other reasonable explanation. Please let me know what you think about this.

If you need any extra info, please let me know! :)

tillig commented 7 years ago

Interesting find. I don't think this combo is as likely as you might think it is, but the root cause you traced it down to (thanks, by the way) makes sense. EnableInterfaceInterceptors switches the instance that's getting returned so it's the proxy object rather than the original object, but OnActivated assumes it's the original object.

Since you can register things as multiple types...

builder.RegisterType<A>()
  .As<IA>()
  .As<IB>();

...the OnActivated wants the component type A to match and doesn't try to cast to one of the service types IA or IB. When the interface interceptor is generated, it doesn't derive from the class (it's interface interceptors not class interceptors) so I see why it fails.

I'm not clear on how to fix that at this point, but a workaround you could try is to make an extension method that doesn't try to cast and just uses object:

public static IRegistrationBuilder<TLimit, TActivatorData, TRegistrationStyle> OnActivatedObject(
  this IRegistrationBuilder<TLimit, TActivatorData, TRegistrationStyle> builder,
  Action<IActivatedEventArgs<object>> handler)
{
  if (handler == null) throw new ArgumentNullException(nameof(handler));
  builder.RegistrationData.ActivatedHandlers.Add(
    (s, e) => handler(new ActivatedEventArgs<object>(e.Context, e.Component, e.Parameters, e.Instance)));
  return builder;
}

That should at least unblock you.

Again, not sure what the better or longer term fix might be. We can't switch OnActivated to just use object by default because that's pretty breaking and having a strongly-typed instance is helpful in the majority case. We can't really cast the object in the event arguments to "whatever you resolved" because a given component may expose several services and those could be very different, making the handler really hard to write with a bunch of if/else logic.

The one thing we've run into where we had trouble with interception before like this was in WCF interception where we had to add a special case for .NET remoting transparent proxies.

I can imagine that one option might be that we add an OnInterceptorActivated or some similar event that will "know" about the interception and behave accordingly. Not saying that's the answer, just one option. Open to suggestions,

bcmedeiros commented 7 years ago

Hi, tillig! Thanks for the fast reply :)

About being blocked, what I did here is to avoid adding the OnActivated handler when we don't need to (we used to add an empty one in most of the cases just by convenience). Thank's for the extension, though, we'll definitely try it :)

About the actual issue, at first sight I thought the cast issue was happening on my handler, not inside Autofac, so I tried this:

 r.OnActivated(a =>
    {
        if (ProxyUtil.IsProxy(a.Instance))
            {
                init((Impl) ProxyUtil.GetUnproxiedInstance(a.Instance));
            }
            else
            {
                init(a.Instance);
            } 
    });

It didn't fix the problem, but I don't think it's a bad idea. Actually, I think this is the proper way to fix it since the handler registered with OnActivated is meant to run directly on the real instance, not being intercepted by any proxy and consequently not being affected by any custom behavior.

What do you think?

tillig commented 7 years ago

We'd have to figure out how to do that in the Autofac.Extras.DynamicProxy library. Autofac core shouldn't know anything about proxies or interceptors.

I'd be curious to see if we have a similar issue with decorators, which is part of core Autofac.

bcmedeiros commented 7 years ago

Maybe creating in core something like a IInstanceResolver that could be implemented and provided by the Autofac.Extras.DynamicProxy module in order to resolve the proper instance? I don't see a way to completely abstract the core about the proxy concept to solve this...

bcmedeiros commented 7 years ago

Hi, tillig.

As I also needed the onRelease(), I ended up with the following extension:

public static class AutofacBug860WorkaroundExtension
    {

        public static IRegistrationBuilder<TLimit, TActivatorData, TRegistrationStyle> 
            OnActivatedProxyAware<TLimit, TActivatorData, TRegistrationStyle>(
            this IRegistrationBuilder<TLimit, TActivatorData, TRegistrationStyle> builder,
            Action<IActivatedEventArgs<TLimit>> handler)
        {
            if (handler == null) throw new ArgumentNullException(nameof(handler));

            builder.RegistrationData.ActivatedHandlers.Add(
                (s, e) => handler(new ActivatedEventArgs<TLimit>(e.Context, e.Component, e.Parameters, ResolveProxy<TLimit>(e.Instance))));
            return builder;
        }

        public static IRegistrationBuilder<TLimit, TActivatorData, TRegistrationStyle>
            OnReleaseProxyAware<TLimit, TActivatorData, TRegistrationStyle>(
                this IRegistrationBuilder<TLimit, TActivatorData, TRegistrationStyle> registration,
                Action<TLimit> releaseAction)
        {
            if (registration == null) throw new ArgumentNullException(nameof(registration));
            if (releaseAction == null) throw new ArgumentNullException(nameof(releaseAction));

            // Issue #677: We can't use the standard .OnActivating() handler
            // mechanism because it creates a strongly-typed "clone" of the
            // activating event args. Using a clone means a call to .ReplaceInstance()
            // on the args during activation gets lost during .OnRelease() even
            // if you keep a closure over the event args - because a later
            // .OnActivating() handler may call .ReplaceInstance() and we'll
            // have closed over the wrong thing.
            registration.ExternallyOwned();
            registration.RegistrationData.ActivatingHandlers.Add((s, e) =>
            {
                var ra = new ReleaseAction<TLimit>(releaseAction, () => ResolveProxy<TLimit>(e.Instance));
                e.Context.Resolve<ILifetimeScope>().Disposer.AddInstanceForDisposal(ra);
            });
            return registration;
        }

        public static TImpl ResolveProxy<TImpl>(object inst)
        {
            if (ProxyUtil.IsProxy(inst))
            {
                return (TImpl) ProxyUtil.GetUnproxiedInstance(inst);
            }
            else
            {
                return (TImpl) inst;
            }
        }

        internal class ReleaseAction<TLimit> : Disposable
        {
            private readonly Action<TLimit> _action;
            private readonly Func<TLimit> _factory;

            public ReleaseAction(Action<TLimit> action, Func<TLimit> factory)
            {
                if (action == null) throw new ArgumentNullException(nameof(action));
                if (factory == null) throw new ArgumentNullException(nameof(factory));

                _action = action;
                _factory = factory;
            }

            protected override void Dispose(bool disposing)
            {
                // Value retrieval for the disposal is deferred until
                // disposal runs to ensure any calls to, say, .ReplaceInstance()
                // during .OnActivating() will be accounted for.
                if (disposing)
                    _action(this._factory());

                base.Dispose(disposing);
            }
        }
    }

Two questions:

  1. Do I really need to copy the whole ReleaseAction in order to create this onReleaseProxyAware() extension?
  2. Any ideas on how to fix it upstream yet?
tillig commented 7 years ago

I'm sorry, I just don't have any answers for you at the moment. There are only two maintainers of Autofac (which includes the 20+ integration packages) and we answer questions on Twitter, StackOverflow, forums, and here. This isn't a paid job, so it's "free time" between the usual 40 - 60 hours/week day job and trying to spend time with our families. There are a lot of open issues across all of these repos, some of which have been open for months or, in the case of Autofac core, years.

I recognize and respect that you have a challenge here. You have indeed found a shortcoming, but coming to an actual solution is not going to be a five minute task. I am currently in the middle of handling two other issues that have come in before this one. What that unfortunately means is this is going to have to wait for something official.

If you can figure out how to enhance either core Autofac or Autofac.Extras.DynamicProxy such that...

...I'm open to suggestions. A PR would be even better. We usually prioritize issues with good quality pull requests higher because there's less work for us and it shows the developer has a vested interest in seeing the problem solved.

Until then, it seems you've found a workaround that unblocks you. We appreciate your patience and understanding as we try to get to everyone's questions and provide the best possible product for our users.

bcmedeiros commented 7 years ago

tillig, first of all, sorry if I sounded like demanding you, I completely understand how opensource works ;) I just asked because you promptly answered, so I thought you might be working on it.

This extension is working well so far, so we don't need to hurry. I'll try to find some time to build a proper PR.

Thank you again for your attention :)

alistairjevans commented 3 years ago

I strongly suspect that this issue has been resolved in v6, because we now have proper controlled ordering in DynamicProxy, where Interface Interceptions happen at the right time for Activated handlers to capture the 'real' instance.

tillig commented 3 years ago

Verified this works in v6.

I tried it with this quick C# script using the same code as in the original post:

#!/usr/bin/env dotnet-script
#r "nuget:Autofac, 6.0.0"
#r "nuget:Autofac.Extras.DynamicProxy, 6.0.0"

using System;
using Autofac;
using Autofac.Extras.DynamicProxy;
using Castle.DynamicProxy;

public interface IA
{
    string Hello();
}

public class A : IA
{
    public string Hello()
    {
        return "hello world!";
    }
}

public class DummyInterceptor : IInterceptor
{
    public void Intercept(IInvocation invocation)
    {
        Console.Write("before ");
        invocation.Proceed();
    }
}

var builder = new ContainerBuilder();

builder.Register(a => new DummyInterceptor());

var r = builder.RegisterType<A>()
    .As<IA>()
                          // Changed to a.Instance to get better info.
    .OnActivated(a => Console.WriteLine(a.Instance.GetType().ToString()))
    .EnableInterfaceInterceptors()
    .InterceptedBy(typeof(DummyInterceptor));

var c = builder.Build();

var ia = c.Resolve<IA>();

Console.WriteLine(ia.Hello());

The console output for this is:

Submission#0+A
before hello world!

The Submission#0 is an artifact of C# script, since there's no main program or namespace, that's a generated one. Point is, it appears this works as expected.