autofac / Autofac.WebApi

ASP.NET Web API integration for Autofac
Other
36 stars 27 forks source link

Change IAutofacActionFilter and similar signatures to Task instead of void. #6

Closed tillig closed 8 years ago

tillig commented 9 years ago

From @alexmg on January 22, 2014 14:21

From felipe.l...@gmail.com on February 28, 2013 13:06:35

What steps will reproduce the problem?

  1. Create and implementation of IAutofacActionFilter
  2. On the implementation of OnActionExecuting decorate it with async
  3. Create a test case for the OnActionExecuting method.
  4. The test completes because I cannot await a void method. What is the expected output? What do you see instead? If there is either an async version of IAutofacActionFilter or it returns a task testing would be possible. The code is behaving as expected I just can't write tests for it. What version of Autofac are you using? On what version of .NET/Silverlight? Autofac 3.0.0 Please provide any additional information below. I can`t use a common IActionFilter because of a requirement to use an component that is InstancePerApiRequest().

Original issue: http://code.google.com/p/autofac/issues/detail?id=411

Copied from original issue: autofac/Autofac#411

tillig commented 9 years ago

From @alexmg on January 22, 2014 14:21

From travis.illig on February 28, 2013 16:23:20

Labels: Module-Integration-WebApi

tillig commented 9 years ago

The IAutofacActionFilter (and related filter interfaces) are not async because the way they're currently interfacing with the system is via an override of the ActionFilterAttribute (and related attributes), which sort of "convert" the async action to synchronous.

In the current [non-async] implementation of IAutofacActionFilter, you can't just decorate it with await - you have to get the Task (or Task<T>) coming back from the async call you're making and request the Task.Result to get the await to happen.

In a different model, what the Autofac integration could do is switch from overriding the attributes and instead directly implementing IActionFilter (or whatever) so the async handling can still be done. Alternately, if we stick with the attribute override, instead of handling OnActionExecuting we could switch to handle OnActionExecutingAsync and behave appropriately.

I think switching the Autofac filters to look/behave more closely with the standard filters would probably be a good way to go, but it'd definitely be a breaking change for people upgrading.

tillig commented 9 years ago

Google group discussion: https://groups.google.com/forum/#!topic/autofac/yWZcc96VB8w

We don't want to break existing people, but we should look at using the async interfaces and marking the synchronous interfaces obsolete. Keep the action filter simple and continue making use of the error handling/processing in ActionFilterAttribute but for other filters, consider just using the filter interface directly.

madmox commented 9 years ago

I would like to see this happen one day as it currently prevents us from using Autofac's filter interfaces. We are consuming async methods in the filters and want to avoid the dirty hacks to run async methods in synchronous ones, so we use service location in our standard Web Api filters to resolve dependencies. Maybe you have a known better way to handle this case?

The ideal would be that Autofac supports IActionFilter / IExceptionFilter / IAuthenticationFilter and IAuthorizationFilter out of the box without the custom interfaces, combined with the custom filter registration mechanism (builder.RegisterWebApiFilterProvider(config) + builder.Register(...).AsWebApiXXXFilterFor<T>()) that allows per request dependency injections in them.

nathan-alden-sr commented 8 years ago

God, I love Autofac. The best DI library out there for .NET. If this one issue could be corrected, it'd be perfect IMO. ActionFilterAttribute supports asynchronous programming, so any Autofac interfaces designed to replace ActionFilterAttribute must do the same. I have an [Authorization] attribute that I inject with an ITokenService implementation that does JWT validation. ITokenService itself must use asynchronous programming, so all its methods are asynchronous as well. As it stands, I am forced to keep using ActionFilterAttribute until this interface supports asynchronous programming.

I agree with @madmox's suggestions. Optimally, Autofac would use as many standard Web API interfaces as possible to allow for maximum interoperability.

tillig commented 8 years ago

Given the security model changes we'll need to make to be compatible with Autofac 4.0 this may be a good time to introduce some of these changes.

Currently all of the time in the project is going to .NET Core support for core Autofac. (That's a lot more painful than originally anticipated.) If anyone has some cycles to dedicate to a PR for this, we'd love the help.

If you do want to PR, try to make some specific/detailed suggestions about exactly what might change and how we'd keep back-compat for existing users where possible. If we chat design first it could help save some code churn.

narciero commented 8 years ago

+1 for async support. Could really use this change

tillig commented 8 years ago

I just pushed Autofac.WebApi2 4.0.0-CI-220 to MyGet (https://www.myget.org/F/autofac/api/v3/index.json) which includes the switch of the filter interfaces to use the Task/async instead of synchronous versions.

Anyone interested in trying it out and letting me know if it works?

tillig commented 8 years ago

I went ahead and pushed 4.0.0-rc3-225 to NuGet with these changes and a few other fixes that have been piling up. I'm going to close the issue with that; if it turns out this isn't actually fixed, let me know and we can re-open.