Filter caching is too aggressive

[rynowak]

We're mistakenly keeping references to filters even when they say they aren't reusable. This creates all kinds of use-after-dispose problems, and will break apps that worked in RC1.

The bug is here: https://github.com/aspnet/Mvc/blob/dev/src/Microsoft.AspNetCore.Mvc.Core/Internal/ControllerActionInvokerCache.cs#L59

The GetFilters method is too granular, the same logic can't work for the 'first time' and for the cached case.

To repro this add a filter like:

    public class MyFilter : IActionFilter, IDisposable
    {
        private bool _isDisposed;
        private ILogger<MyFilter> _logger;

        public MyFilter(ILogger<MyFilter> logger)
        {
            _logger = logger;

            _logger.LogError($"Created {RuntimeHelpers.GetHashCode(this)}");
        }

        public void Dispose()
        {
            _isDisposed = true;
            _logger.LogError($"Disposed {RuntimeHelpers.GetHashCode(this)}");

        }

        public void OnActionExecuted(ActionExecutedContext context)
        {
        }

        public void OnActionExecuting(ActionExecutingContext context)
        {
            _logger.LogError($"Executing {RuntimeHelpers.GetHashCode(this)}");
            if (_isDisposed)
            {
                _logger.LogError($"Use after dispose {RuntimeHelpers.GetHashCode(this)}");
                throw new Exception();
            }
        }
    }

Register it as a scoped service: services.AddScoped<MyFilter, MyFilter>();

Add it as a ServiceFilter:

        [ServiceFilter(typeof(MyFilter))]
        public IActionResult Index()
        {
            return View();
        }

You'll see an exception on the third request.

Output is like

info: Microsoft.AspNetCore.Hosting.Internal.WebHost[1]
      Request starting HTTP/1.1 GET http://localhost:5000/
fail: MvcSandbox.MyFilter[0]
      Created 25517075
fail: MvcSandbox.MyFilter[0]
      Executing 25517075
info: Microsoft.AspNetCore.Mvc.Internal.ControllerActionInvoker[1]
      Executing action method MvcSandbox.Controllers.HomeController.Index (MvcSandbox) with arguments () - ModelState is Valid'
info: Microsoft.AspNetCore.Mvc.ViewFeatures.Internal.ViewResultExecutor[1]
      Executing ViewResult, running view at path /Views/Home/Index.cshtml.
info: Microsoft.Extensions.DependencyInjection.DataProtectionServices[0]
      User profile is available. Using 'C:\Users\rynowak\AppData\Local\ASP.NET\DataProtection-Keys' as key repository and Windows DPAPI to encrypt keys at rest.
info: Microsoft.AspNetCore.Mvc.Internal.MvcRouteHandler[2]
      Executed action MvcSandbox.Controllers.HomeController.Index (MvcSandbox) in 6722.2956ms
fail: MvcSandbox.MyFilter[0]
      Disposed 25517075
info: Microsoft.AspNetCore.Hosting.Internal.WebHost[2]
      Request finished in 7122.4393ms 200 text/html; charset=utf-8
info: Microsoft.AspNetCore.Hosting.Internal.WebHost[1]
      Request starting HTTP/1.1 GET http://localhost:5000/
fail: MvcSandbox.MyFilter[0]
      Created 48094426
fail: MvcSandbox.MyFilter[0]
      Executing 48094426
info: Microsoft.AspNetCore.Mvc.Internal.ControllerActionInvoker[1]
      Executing action method MvcSandbox.Controllers.HomeController.Index (MvcSandbox) with arguments () - ModelState is Valid'
info: Microsoft.AspNetCore.Mvc.ViewFeatures.Internal.ViewResultExecutor[1]
      Executing ViewResult, running view at path /Views/Home/Index.cshtml.
info: Microsoft.AspNetCore.Mvc.Internal.MvcRouteHandler[2]
      Executed action MvcSandbox.Controllers.HomeController.Index (MvcSandbox) in 4.4185ms
fail: MvcSandbox.MyFilter[0]
      Disposed 48094426
info: Microsoft.AspNetCore.Hosting.Internal.WebHost[2]
      Request finished in 5.8475ms 200 text/html; charset=utf-8
info: Microsoft.AspNetCore.Hosting.Internal.WebHost[1]
      Request starting HTTP/1.1 GET http://localhost:5000/
fail: MvcSandbox.MyFilter[0]
      Executing 48094426
fail: MvcSandbox.MyFilter[0]
      Use after dispose 48094426

Notice that the object with ID 48094426 gets used twice

[kichalla]

d6a9068

[Eilon]

@kichalla safe to close? Is this merged into dev as well?

[kichalla]

Hasn't been merged to dev yet. Doing a local build now. Will close once I merge it.

[Eilon]

Thanks!