Bad async/await implementation causing loss of execution context.

[dtabuenc]

Very commonly, filters are used to provide execution context around an action. For example, wrapping an action method with a TransactionScope, LogContext, AsyncLocal variables, etc. When operating in an asynchronous environment using async/await, many of these things flow through the action through the use of the ExecutionContext.

The current implementation of ActionFilterWrapper simply iterates over a collection of filters and naively calls await for each filter in turn. The problem with this is that whenever await is used any state set on the execution context of the underlying filter will be lost since await will restore the current execution context upon the completion of the underlying Task. This makes it impossible to do the kind of things mentioned above, such as using TransactionScope with the TransactionScopeAsyncFlowOption.Enabled, or setting any AsyncLocal variables.

In order to work property, the execution of asynchronous filters needs to be chained. An example of this can be seen in how plain AttributeFilters are executed by the framework:

 Func<Task<HttpResponseMessage>> result = innerAction;
            for (int i = filters.Length - 1; i >= 0; i--)
            {
                IActionFilter filter = filters[i];
                Func<Func<Task<HttpResponseMessage>>, IActionFilter, Func<Task<HttpResponseMessage>>>
                    chainContinuation = (continuation, innerFilter) =>
                    {
                        return () => innerFilter.ExecuteActionFilterAsync(actionContext, cancellationToken,
                            continuation);
                    };
                result = chainContinuation(result, filter);
            }
            return result;

I would like to submit a pull request, but wanted to check before I do if this makes sense to everyone. This has a small potential under rare circumstances to be a breaking change, but as it stands I can't migrate from 3.x to 4.x because almost all of the filters we use would require proper async support without losing execution context.

[tillig]

I think the idea here seems reasonable. If you have a PR, we'd be happy to check it out. Do what you can to ensure no interfaces change, though I recognize the behavior difference may cause discrepancies in expectations for some folks. We'd likely rev this by 0.1 as a "new feature."

[alistairjevans]

Assuming that I've understood the original poster correctly (which is that async context is not maintained between OnActionExecutingAsync and OnActionExecutedAsync, so AsyncLocal cannot be used in an IAutofacActionFilter), then I don't believe this is something we can change without replacing the entire way that the IAutofacActionFilter works and is laid out.

Please correct me if I've misunderstood the raised issue, but my thoughts on this are as follows:

IAutofacActionFilter is not the same as IActionFilter, which takes in the next part of the request pipeline to execute as a continuation; IAutofacActionFilter behaves more like a class deriving from ActionFilterAttribute.

The same behaviour as the OP reported is also apparent if you create a non-autofac filter derived from ActionFilterAttribute when not using Autofac; you can't use AsyncLocal between OnActionExecutingAsync and OnActionExecutedAsync there either.

I believe this is because the ActionFilterAttribute code itself is the one responsible for establishing the async context when it calls await on the OnActionExecutingAsync method.

IAutofacActionFilter would have to change to implement IActionFilter, and the ActionFilterWrapper would have to be replaced entirely, for this to work.

---

Personally, considering that you can still maintain state between the two methods by registering the Autofac filter as InstancePerRequest and just storing local state in the filter class (so AsyncLocal isn't needed), plus the size of the break in API signature that would result (i.e. complete replacement), I feel like we shouldn't fix this.

If the OP has more information about a specific case, or I've completely misunderstood, then do let me know.

[alistairjevans]

@tillig, would you mind just checking if you're ok with me closing this as won't fix based on my comments?

[tillig]

The use case proposed sounds less about maintaining state within the filter and more about being able to affect other async state.

For example, Serilog has a notion of LogContext where you can add context to log messages by accessing a static, AsyncLocal sort of variable.

What I'm reading here is that if you try to do this from inside an Autofac filter, that doesn't work.

• I haven't actually tried this to verify that's the case. Async/await can get tricky and it's possible this is failing due to other application code not behaving.
• I don't know how the ASP.NET Web API stack actually handles it That'll require some spelunking. It looks like @dtabuenc found it but I'm not sure where it is or if there's a way to "hook in" or something like that.
• This Autofac integration code was written a long time ago. It's very possible we didn't get it 100% right and maybe it does need some pretty significant updating. I don't think that's out of the question, it's just not five minutes of work.

Very light looking on my part shows we could build the continuation like shown in the OP by adding that into the wrapper instead of just doing foreach around the await. Maybe that'd work? Maybe I'm oversimplifying.

This might be something worth bringing the other @autofac/contributors in to help look at; I'm a bit swamped at the moment so I can't dive in as deep as I might need to.

If it can't be done, I'm fine with closing it.

If it can be done without changing public interfaces, make it a 0.1 semver increase.

If it has to break a public interface, it'll be a 1.0 major version increase, in which case if there are other breaking changes we need to make for any other pending issues, let's try to roll them all in at once.

[alistairjevans]

I went pretty deep in my investigation on this one, and replicated the scenario @dtabuenc outlined. It's not that it just doesn't work inside an Autofac filter, it doesn't work inside any filter derived from the WebAPI ActionFilterAttribute.

The async context that's causing the problem is started by the ActionFilterAttribute from which the Autofac wrapper derives, so it's not under our control. The example @dtabuenc gave is from the code that invokes the set of all IActionFilter instances, rather than inside the ActionFilterAttribute itself.

To fix this, we'd need to replace the AutofacFilterWrapper with something that implements IActionFilter directly, rather than deriving from ActionFilterAttribute. This would probably require a pretty significant overhaul and a definite change in the public interface, because IAutofacFilter would have to be replaced.

So it's not that it can't be done, but more that I wonder if there's any point in making such a major change in an old integration, when there's been no other interaction with this issue since it was originally raised, and no obvious evidence of people struggling with this issue?

[tillig]

Well, there are three +1 on the original post, so it's not a total zero-affected issue, but it's also true that this is an older integration, so it's not as high a priority.

If we have some time to dive in here and do something to fix this up, let's do it. This is one of the reasons we put the call for owners to come out - some of these things that seem small really aren't small but the original two or three folks on here didn't have time to dive in and actually address these longer-running items. (There are a bunch of these in core Autofac, for example - stuff that seems like it's affecting just a few people but probably should be fixed... and will require some pretty hefty work to get it fixed.)

That said, I wouldn't put a super high priority on it, either, which is why I didn't close it. I think something can be done but in context of all the other things pending this maybe isn't the #1 thing to grab.

If you've got some time, maybe branch and see what we could do to address it. If you have other issues to tackle, go ahead and grab those and come back to this later.

[alistairjevans]

Ok, understood, I think I know what's needed, since there's a breaking change I may loop in #17 at the same time, since both will require a fairly major overhaul.

[alistairjevans]

I have the fix for this issue in my fork, but it is dependent on the changes in PR #43.

You can see the changes in a PR on my fork, alistairjevans#1.

[kbilsted]

Glad to see this fixed :-)