PropertiesAutowired not working when EnableInterfaceInterceptors and AllowCircularDependencies

[masterxml]

I use Autofac DynamicProxy2 do AOP logging. If I register like this:

builder.RegisterAssemblyTypes(assemblys.ToArray()).Where(t => t.Name.EndsWith("ServiceImpl"))
                .AsImplementedInterfaces()
                .InstancePerLifetimeScope()
                .PropertiesAutowired()
                .EnableInterfaceInterceptors()
                .InterceptedBy(typeof(LogInterceptor));
            ;

then ValuesServiceImpl instance can be auto wire to property, like this:

public class ValuesController : ApiController
    {
        public IValuesService ValuesService { get; set; }
    }

But if I use PropertiesAutowired(PropertyWiringOptions.AllowCircularDependencies)

builder.RegisterAssemblyTypes(assemblys.ToArray()).Where(t => t.Name.EndsWith("ServiceImpl"))
                    .AsImplementedInterfaces()
                    .InstancePerLifetimeScope()
                    .PropertiesAutowired(PropertyWiringOptions.AllowCircularDependencies)
                    .EnableInterfaceInterceptors()
                    .InterceptedBy(typeof(LogInterceptor));

then PropertiesAutowired not working, ValuesService property is null.

In my project I must allow Circular Dependencies, and I prefer use EnableInterfaceInterceptors instead of EnableClassInterceptors

[tillig]

We'll see what we can do to track this down, but we're a bit bogged down by .NET Core fixes right now, so it may be a bit. (We accept PRs!) As soon as that lets up some, we'll look into this. Thanks!

[tillig]

Cross-posted to StackOverflow.

[masterxml]

Hi, @tillig , After research source code, I found this: When enable circular dependency, RegistrationBuilder inject properties when Activated. but the EnableInterfaceInterceptors set InterfaceProxy on Activating.

Any advice will be appreciated.

[RaymondHuy]

Hi @tillig after investigating about this issue, I see that the reason is about when using .PropertiesAutowired(PropertyWiringOptions.AllowCircularDependencies) it will add an ActivatedEvent to inject properties to instance however the instance has been changed when handling ActivatingEvent raised by EnableInterfaceInterceptors so its property is null.

My approach is to change EnableInterfaceInterceptors to raise ActivatedEvent and make its Instance property to be settable(Currently it is read-only field https://github.com/autofac/Autofac/blob/develop/src/Autofac/Core/ActivatedEventArgs.cs#L74).

I aslo has one more concern regarding the issue autofac/Autofac#860. Currently I see the order of fluent API is very important. For example:

builder.RegisterAssemblyTypes(assemblys.ToArray()).Where(t => t.Name.EndsWith("ServiceImpl"))
                    .AsImplementedInterfaces()
                    .InstancePerLifetimeScope()
                    .PropertiesAutowired()   //1
                    .OnActivated(a => Console.WriteLine( a.GetType().ToString() ))   //2
                    .EnableInterfaceInterceptors()       //3
                    .InterceptedBy(typeof(LogInterceptor));

if we swap the order of //1 //2 //3 with each others it can result to different output because it affects the order of raising event. Somehow I see it doesn't make sense so my additional suggestion is to make the method EnableInterfaceInterceptors return void instead of a fluent API so that we can ensure the interception will run as a last event.

What do you think about it ?

[tillig]

Sounds like this overlaps a lot with autofac/Autofac#860, which is also about EnableInterfaceInterceptors() and event ordering.

I've noodled on this for the last day or so since you made some proposals. In an ideal world I think the fluent builder syntax should just "do the right thing." This skips over a lot of important details to be sure, but the idea is that the order technically shouldn't matter.

I'm afraid of changing EnableInterfaceInterceptors to be void for a few reasons:

• It'd be breaking big time, and while I get that some things do need to be breaking changes, it'd be really nice to avoid it.
• It will change the fluent semantics. If it returns void, does that mean the InterceptedBy call would have to be before EnableInterfaceInterceptors? If so, that... reads really oddly, like Yoda writing the fluent grammar.
• It seems like we could very easily run into one of these ordering issues again, and if the answer is "change the thing that needs to be last to void" then we're going to have a lot of void methods.

Things that randomly come across my head which are probably not good ideas but might spur some inspiration or additional other ideas...

• What if we had more events in the pipeline? A richer event chain, like pre-activating, activating, post-activating, pre-activated, activated, post-activated... or whatever. Maybe better names. Could we control things in the lifecycle ordering better with more events?
• What if we could declare event handler order? Like action filters in ASP.NET MVC where you can set an order value and before they get run they get sorted by order. I don't think that's something that comes "out of the box" with events but maybe we could do something internally that makes it seem like it's ordered?

Perhaps @alexmg , @alistairjevans , or @alsami have additional ideas?

Or maybe I'm too focused on it being an ordering issue?

[alsami]

Or maybe I'm too focused on it being an ordering issue?

If you think about it, there are many cases out there where the order makes a difference.

Samples for using middleware in ASP.NET Core:

public void Configure(IApplicationBuilder app)
{
   app.UseAuthentication();
   app.UseMvc(); // or app.UseRouting();
}

Here, app.UseAuthentication() must be called ahead of app.UseMvc();.

Another sample is adding filters to the ASP.NET Core pipeline for instance implementations of IExceptionFilter:.

public void ConfigureServices(IServiceCollection services)
{
   // or services.AddMvc();
   services.AddControllers(options => 
   {
        options.Filters.Add<UnhandledExceptionFilter>();        
        options.Filters.Add<ResourceNotFoundExceptionFilter>(); 
   });
}

Here again, if we don't define the order of the filters, which was introduced in 2.2 of ASP.NET Core as far as I remember, we could have a problem when registering UnhandledExceptionFilter after ResourceNotFoundExceptionFilter.

Same thing happens for every modern IoC framework that allows multiple registrations for a specific interface. Whatever you register last, is being resolved first, unless you use IEnumerable<T>, then of course it doesn't matter.

What I am trying to say: The order matters almost everywhere.

It does not only matter in Autofac. While it might be frustrating for users to end up with weird side effects because of implicit behavior, this is not a new thing.

Maybe documenting that implicit behaviors as good as possible is better than introducing breaking-changes or rebuilding the internals of how events are being raised and when.

--- Edit ---

Treating each event like a request to be processed allows a given handler to consider the before/after the 'next' event handler, with the last stage of the pipeline being the Autofac Core behaviour. You can see an implementation of this in AutoStep, a project I'm working on right now.

Interesting. Just thought about something after you have posted that code. Maybe a state-machine could be helpful solving the problem on how and when events are being raised.

[alistairjevans]

My two cents:

My experience of writing extensible systems has shown me that having a specific order value assigned to an event handler is an eventual recipe for disaster (even if it fixes the immediate problem). When two extension writers both set the same order value, you're back to square one. Or there's a competing race to have the highest/lowest order value. It becomes something of a nightmare.

The most effective event-handling solution I've found in such systems is to have a generous number of clearly defined events that are strongly ordered, with event handlers attached to each that are not ordered. Autofac already has a number of such events (Preparing, Activating, Activated, etc), but they could be extended/cleaned up.

An Alternative

An alternate approach/improvement (in my opinion) to the event handling model when using 'declarative' event ordering (like we appear to have in the fluent set-up, and @alsami points out is pretty far-reaching already) might be to use a middleware-style event pipeline model for event handling.

Treating each event like a request to be processed allows a given handler to consider the before/after the 'next' event handler, with the last stage of the pipeline being the Autofac Core behaviour. You can see an implementation of this in AutoStep, a project I'm working on right now.

In such a model, the three Autofac event 'types' would be Preparation, Activation and Release.

An event handler would look like:

void EventHandler(ActivationEventArgs args, Action<ActivationEventArgs> next) 
{
         // Before Activation
        next(args);
        // After Activation
}

A stage in the event pipeline simply doesn't call 'next' if it doesn't want the default behaviour (or other handlers) to execute.

[tillig]

It's interesting that you mention a pipeline approach. I've thought about something similar but didn't really know how to make it work in the context of the existing system. It could be this is the solution.

[alistairjevans]

Treating the ResolveRequest as an actual request would add a lot of power, but it would be a significant change to a lot of the Autofac Internals.

It's the kind of thing that would need to be wholly embraced. Not sure we can just add it on in a simple way.

[RaymondHuy]

Thanks for all your suggestions. Is it ok if I go with the approach: replace the ActivatingHandler (https://github.com/autofac/Autofac.Extras.DynamicProxy/blob/3eca5130bbfad59da1e99fa856e8ef6369fd888d/src/Autofac.Extras.DynamicProxy/RegistrationExtensions.cs#L191) with ActivatedHandler. It resuls to modify parameter instance in https://github.com/autofac/Autofac.Extras.DynamicProxy/blob/3eca5130bbfad59da1e99fa856e8ef6369fd888d/src/Autofac.Extras.DynamicProxy/RegistrationExtensions.cs#L191 to have ref keyword and Instance property in ActivatedEvent need to be settable ? If it is the point I will create a PR target v6 branch.

[tillig]

I'm really torn on this. We've even mentioned in docs that the time to switch the current instance is during OnActivating so doing it later, while it solves the problem, seems like we're going against our own guidance. That said, it's a legit way to fix the issue, so I'm not totally against it, just slightly hesitant.

If it will have to wait for a 6.0 release anyway would it make sense to see what we can do to improve the overall resolution pipeline, possibly either adding new events or updating the event handling to be more pipeline-ish?

Possibly related: if we change the way the resolution pipeline works, would that affect our ability to be more linker-safe or generating resolve ops from build time? Thinking about this @alistairjevans Twitter thread. (Also, I don't mean to sideline this issue by going into pipeline design discussions; let me know if we should take that elsewhere.)

[alistairjevans]

I'm in favour of looking at a pipeline approach to resolving more generally, but we want to be absolutely sure that will fix the problem we have here.

The issue we have here is the interactions of the following methods:

 .PropertiesAutowired()   //1
 .OnActivated(a => Console.WriteLine( a.GetType().ToString() ))   //2
 .EnableInterfaceInterceptors()       //3
 .InterceptedBy(typeof(LogInterceptor));

Here's what this the same chain would 'output' in a middleware approach:

 .PropertiesAutowired()                  // Adds AutoWirePropertiesMiddleware
 .OnActivated(a => {})                   // Adds OnActivatedEventHandlerMiddleware (to call delegate)  
 .EnableInterfaceInterceptors()          // Adds InterfaceInterceptorsMiddleware 
 .InterceptedBy(typeof(LogInterceptor)); // Configures InterfaceInterceptorsMiddleware

In this event model, the order of pseudo-behaviour would look something like this:

AutoWirePropertiesMiddleware
    next(ctxt) 

        ➡ OnActivatedEventHandlerMiddleware
            next(ctxt)

                ➡ InterfaceInterceptorsMiddleware
                   // Does not call next (the default autofac resolve)
                   ctxt.Instance = proxy

            event.Invoke(ctxt)

InjectProperties(ctxt.Instance)

That will work, but more generally, a naive pipeline approach does make the registration approach even more dependent on order and my gut worries it may put too much onus on developers to order the extension methods correctly.

We might be able to mitigate that by defining pipeline 'groups' that tries to apply some sort of order, not sure.

One big gain of switching to a middleware approach, to reference your latter point @tillig, would be that when we get to the code generators being available in .NET 5 it would be much easier to generate static pipelines for each service, which would hopefully reduce the time we spend looking up things in dictionaries.

We should probably discuss the pipelines elsewhere if we want to go into it in more depth.

[tillig]

v6 is moving full-steam-ahead and the pipeline mechanism has been implemented. It would be interesting to see if a small example repro could pass using the new bits (currently in the develop branch). Since interface interception is Autofac.Extras.DynamicProxy if we ended up with a unit test it'd be there rather than in core Autofac; but a small repro repo with a working example would also be OK just to see if we can validate this out.

[alistairjevans]

I'll take a look at putting a test in the DynamicProxy repo and come back to you.

[alistairjevans]

Ok, so I have recreated the issue in unit tests in the dynamic proxy project. The property injection mode that permits circular dependencies still fails.

However, this is actually due to a separate problem in v6 that I've just figured out, whereby in Circular Dependency Permitted mode, property injection doesn't run until the end of the entire operation. That can't really fly, because it causes concurrency problems. I've raised a separate issue for that (https://github.com/autofac/Autofac/issues/1200).

public class InterfaceInterceptionWithPropertyInjectionFixture
{
    [Fact]
    public void InterfaceInterceptorsSupportPropertyInjection()
    {
        var builder = new ContainerBuilder();
        builder.RegisterType<StringMethodInterceptor>();

        builder.RegisterType<OtherImpl>().As<IOtherService>();

        builder
            .RegisterType<InterceptableWithProperty>()
            .PropertiesAutowired()
            .EnableInterfaceInterceptors()
            .InterceptedBy(typeof(StringMethodInterceptor))
            .As<IPublicInterface>();
        var container = builder.Build();
        var obj = container.Resolve<IPublicInterface>();

        Assert.NotNull(obj.GetServiceProperty());
        Assert.Equal("intercepted-PublicMethod", obj.PublicMethod());
    }

    [Fact(Skip = "https://github.com/autofac/Autofac/issues/758")]
    public void InterfaceInterceptorsSupportCircularPropertyInjection()
    {
        var builder = new ContainerBuilder();
        builder.RegisterType<StringMethodInterceptor>();

        builder.RegisterType<OtherImpl>().As<IOtherService>();

        builder
            .RegisterType<InterceptableWithProperty>()
            .As<IPublicInterface>()
            .PropertiesAutowired(PropertyWiringOptions.AllowCircularDependencies)
            .EnableInterfaceInterceptors()
            .InterceptedBy(typeof(StringMethodInterceptor));
        var container = builder.Build();
        var obj = container.Resolve<IPublicInterface>();

        Assert.NotNull(obj.GetServiceProperty());
        Assert.Equal("intercepted-PublicMethod", obj.PublicMethod());
    }

    public interface IOtherService
    {
    }

    public class OtherImpl : IOtherService
    {
    }

    public interface IPublicInterface
    {
        string PublicMethod();

        IOtherService GetServiceProperty();
    }

    public class InterceptableWithProperty : IPublicInterface
    {
        public IOtherService ServiceProperty { get; set; }

        public IOtherService GetServiceProperty() => ServiceProperty;

        public string PublicMethod()
        {
            throw new NotImplementedException();
        }
    }

    private class StringMethodInterceptor : IInterceptor
    {
        public void Intercept(IInvocation invocation)
        {
            if (invocation.Method.ReturnType == typeof(string))
            {
                invocation.ReturnValue = "intercepted-" + invocation.Method.Name;
            }
            else
            {
                invocation.Proceed();
            }
        }
    }
}

[tillig]

Should we keep this issue open or focus on autofac/Autofac#1200 as the root cause?

[alistairjevans]

I'd suggest moving this issue to the DynamicProxy repo. We understand the Autofac implications pretty well (and tracked in autofac/Autofac#1200), but everything else is very DynamicProxy-specific.

[tillig]

Sounds good.