PropertiesAutowired not working when EnableInterfaceInterceptors and AllowCircularDependencies #40
Comments
|
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! |
|
Hi, @tillig , After research source code, I found this: Any advice will be appreciated. |
|
Hi @tillig after investigating about this issue, I see that the reason is about when using My approach is to change 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: 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 What do you think about it ? |
|
Sounds like this overlaps a lot with autofac/Autofac#860, which is also about 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
Things that randomly come across my head which are probably not good ideas but might spur some inspiration or additional other ideas...
Perhaps @alexmg , @alistairjevans , or @alsami have additional ideas? 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, Another sample is adding filters to the ASP.NET Core pipeline for instance implementations of 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 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 What I am trying to say: The order matters almost everywhere. It does not only matter in 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 ---
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. |
|
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 AlternativeAn 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. 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. |
|
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. |
|
Treating the 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. |
|
Thanks for all your suggestions. Is it ok if I go with the approach: replace the ActivatedHandler. It resuls to modify parameter instance in ref keyword and Instance property in ActivatedEvent need to be settable ? If it is the point I will create a PR target v6 branch.
|
|
I'm really torn on this. We've even mentioned in docs that the time to switch the current instance is during 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.) |
|
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 InterfaceInterceptorsMiddlewareIn this event model, the order of pseudo-behaviour would look something like this: 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. |
|
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 |
|
I'll take a look at putting a test in the DynamicProxy repo and come back to you. |
|
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 (autofac/Autofac#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();
}
}
}
} |
|
Should we keep this issue open or focus on autofac/Autofac#1200 as the root cause? |
|
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. |
|
Sounds good. |
I use Autofac DynamicProxy2 do AOP logging. If I register like this:
then ValuesServiceImpl instance can be auto wire to property, like this:
But if I use PropertiesAutowired(PropertyWiringOptions.AllowCircularDependencies)
then PropertiesAutowired not working, ValuesService property is null.
In my project I must allow Circular Dependencies, and I prefer use EnableInterfaceInterceptors instead of EnableClassInterceptors
The text was updated successfully, but these errors were encountered: