Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[4.9.3] OutputCacheAttribute causing issues #1013

Closed
xyanide opened this issue Aug 7, 2019 · 10 comments · Fixed by #1021
Closed

[4.9.3] OutputCacheAttribute causing issues #1013

xyanide opened this issue Aug 7, 2019 · 10 comments · Fixed by #1021

Comments

@xyanide
Copy link

@xyanide xyanide commented Aug 7, 2019

After updating to 4.9.3 (from 4.9.2) i'm getting the following error on controller actions with the output cache attribute:
[OutputCache(NoStore = true, Duration = 0, VaryByParam = "*")]

The error thrown is:

[ArgumentException: Cannot bind to the target method because its signature or security transparency is not compatible with that of the delegate type.]   
System.Reflection.RuntimeMethodInfo.CreateDelegateInternal(Type delegateType, Object firstArgument, DelegateBindingFlags bindingFlags, StackCrawlMark& stackMark) +14797604
   System.Reflection.RuntimeMethodInfo.CreateDelegate(Type delegateType) +43
   Autofac.Core.Activators.Reflection.AutowiringPropertyInjector.MakeFastPropertySetter(PropertyInfo propertyInfo) in C:\projects\autofac\src\Autofac\Core\Activators\Reflection\AutowiringPropertyInjector.cs:141
   System.Collections.Concurrent.ConcurrentDictionary`2.GetOrAdd(TKey key, Func`2 valueFactory) +87
   Autofac.Core.Activators.Reflection.AutowiringPropertyInjector.InjectProperties(IComponentContext context, Object instance, IPropertySelector propertySelector, IEnumerable`1 parameters) in C:\projects\autofac\src\Autofac\Core\Activators\Reflection\AutowiringPropertyInjector.cs:98
   Autofac.ResolutionExtensions.InjectProperties(IComponentContext context, TService instance) in C:\projects\autofac\src\Autofac\ResolutionExtensions.cs:59
   Autofac.Integration.Mvc.AutofacFilterProvider.GetFilters(ControllerContext controllerContext, ActionDescriptor actionDescriptor) +191
   System.Web.Mvc.FilterProviderCollection.GetFilters(ControllerContext controllerContext, ActionDescriptor actionDescriptor) +165
   System.Web.Mvc.ControllerActionInvoker.GetFilters(ControllerContext controllerContext, ActionDescriptor actionDescriptor) +55
   System.Web.Mvc.Async.AsyncControllerActionInvoker.BeginInvokeAction(ControllerContext controllerContext, String actionName, AsyncCallback callback, Object state) +243
   System.Web.Mvc.<>c.<BeginExecuteCore>b__152_0(AsyncCallback asyncCallback, Object asyncState, ExecuteCoreState innerState) +45
   System.Web.Mvc.Async.WrappedAsyncVoid`1.CallBeginDelegate(AsyncCallback callback, Object callbackState) +73
   System.Web.Mvc.Async.WrappedAsyncResultBase`1.Begin(AsyncCallback callback, Object state, Int32 timeout) +163
   System.Web.Mvc.Controller.BeginExecuteCore(AsyncCallback callback, Object state) +789
   System.Web.Mvc.Async.WrappedAsyncResultBase`1.Begin(AsyncCallback callback, Object state, Int32 timeout) +163
   System.Web.Mvc.Controller.BeginExecute(RequestContext requestContext, AsyncCallback callback, Object state) +633
   System.Web.Mvc.<>c.<BeginProcessRequest>b__20_0(AsyncCallback asyncCallback, Object asyncState, ProcessRequestState innerState) +99
   System.Web.Mvc.Async.WrappedAsyncVoid`1.CallBeginDelegate(AsyncCallback callback, Object callbackState) +73
   System.Web.Mvc.Async.WrappedAsyncResultBase`1.Begin(AsyncCallback callback, Object state, Int32 timeout) +163
   System.Web.Mvc.MvcHandler.BeginProcessRequest(HttpContextBase httpContext, AsyncCallback callback, Object state) +544
   System.Web.CallHandlerExecutionStep.System.Web.HttpApplication.IExecutionStep.Execute() +1122
   System.Web.HttpApplication.ExecuteStepImpl(IExecutionStep step) +213
   System.Web.HttpApplication.ExecuteStep(IExecutionStep step, Boolean& completedSynchronously) +131

Reverting to 4.9.2 fixes the issue.
My autofac dependencies are:

id="Autofac" version="4.9.3"
id="Autofac.Mvc5" version="4.0.2"
id="Autofac.Mvc5.Owin" version="4.0.1"
id="Autofac.Owin" version="4.2.0"
.NET 4.7.2

@alistairjevans

This comment has been minimized.

Copy link
Member

@alistairjevans alistairjevans commented Aug 18, 2019

Hi @xyanide, are you able to provide a simple project that recreates this? Just to speed up the investigation. Thanks.

@xyanide

This comment has been minimized.

Copy link
Author

@xyanide xyanide commented Aug 19, 2019

Hi @xyanide, are you able to provide a simple project that recreates this? Just to speed up the investigation. Thanks.

I haven't been able to pinpoint exactly where the issue lies, but have narrowed it down a bit:

  • Removing "builder.RegisterFilterProvider();" removed the error
  • The error is located on line 147 of file AutowiringPropertyInjector (after updating to 4.9.4, and still occurs in this version)
  • It succeeds in creating a generic type of the "OutputCacheAttribute" and parametertype "ObjectCache", but fails when creating a delegate with the error mentioned in the TS.
  • The file in question has been altered a lot since version 4.9.2: 9071c4d#diff-669e117b9fad52f84c8fd3f63e952db8
  • We're also using Microsoft.AspNet.OutputCache.OutputCacheModuleAsync, version="1.0.2", but this didn't seem to make a difference.

Furthermore I created a test project but wasn't able to reproduce the issue there yet. The source project is fairly large so hard to identify where it might go wrong.

@alistairjevans

This comment has been minimized.

Copy link
Member

@alistairjevans alistairjevans commented Aug 27, 2019

OK @xyanide, I think I've figured out what's going on. Unless I'm mistaken, do you have a registration somewhere of a custom ObjectCache implementation? Something like:

builder.Register(c => new CustomObjectCache("local.cache")).As<ObjectCache>().SingleInstance();

With that, I get a recreate of your issue.

What is happening is that Autofac is attempting to inject a dependency onto a static property, specifically ChildActionCache. It tries to create a delegate for the property's setter, and fails because it's not an instance method (static properties have static setter methods).

I actually think Autofac has always injected that dependency, the change in 4.9.3 is that a delegate is created for fast subsequent setting of the property, which is where everything falls down.

Personally, it feels a bit weird that Autofac would be willing to inject service instances onto static properties.

If we do prevent static properties from being injected (which feels like the right behaviour), I fear it might count as a breaking change...from what I can tell, the only way to get a singleton ObjectCache implementation injected into OutputCacheAttribute is through that static property. Other usecases may also be accidentally be relying on this sort of behaviour.

@tillig or @alexmg, does Autofac need to be able to inject onto static properties? If yes, then I can update MakeFastPropertySetter to handle the static property, but if no, then I'll just filter out static properties from the list of potential injection properties.

@tillig

This comment has been minimized.

Copy link
Contributor

@tillig tillig commented Aug 27, 2019

No, I don't think static properties are injectable. Too many shoot-yourself-in-the-foot situations where something like a per-request value could be set on a static thing.

@alistairjevans

This comment has been minimized.

Copy link
Member

@alistairjevans alistairjevans commented Aug 27, 2019

I agree that it would lead to much shooting of feet, but prior to 4.9.3, Autofac's AutowiringPropertyInjector certainly seems to do precisely that. The OutputCacheAttribute does get an Autofac service injected onto its static ChildActionCache property.

image

Assuming we're happy to put in this potentially breaking change, I guess it's one for 5.0? Currently in 4.9.4 anyone using static property injection with the reflection activator will be getting this exception.

@tillig

This comment has been minimized.

Copy link
Contributor

@tillig tillig commented Aug 27, 2019

5.0 works for me.

@alistairjevans

This comment has been minimized.

Copy link
Member

@alistairjevans alistairjevans commented Aug 27, 2019

Ok, I'll get a PR sorted.

@xyanide

This comment has been minimized.

Copy link
Author

@xyanide xyanide commented Aug 28, 2019

@alistairjevans

You are correct, we are using a custom objectcache implementation, thanks for digging into it!
Is there a planned date for 5.0?

@tillig

This comment has been minimized.

Copy link
Contributor

@tillig tillig commented Aug 28, 2019

There is no date for 5.0 at this time.

@alistairjevans

This comment has been minimized.

Copy link
Member

@alistairjevans alistairjevans commented Aug 29, 2019

@xyanide, just a note; once this change is available, the OutputCache attribute won't be populated with your custom ObjectCache. Were you relying on this behaviour? If so you'll need to initialise the ChildActionCache manually.

@tillig tillig closed this in #1021 Aug 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

3 participants
You can’t perform that action at this time.