[Fixes #4616] Filter caching is too aggressive

Author: kichalla

src/Microsoft.AspNetCore.Mvc.Core/Internal/ControllerActionInvokerCache.cs

@@ -56,7 +56,20 @@ public ControllerActionInvokerState GetState(ControllerContext controllerContext
             Entry cacheEntry;
             if (cache.Entries.TryGetValue(actionDescriptor, out cacheEntry))
             {
-                filters = GetFilters(controllerContext, cacheEntry.FilterItems);
+                // Deep copy the cached filter items as filter providers could modify them
+                var filterItems = new List<FilterItem>(cacheEntry.FilterItems.Count);
+                for (var i = 0; i < cacheEntry.FilterItems.Count; i++)
+                {
+                    var filterItem = cacheEntry.FilterItems[i];
+                    filterItems.Add(
+                        new FilterItem(filterItem.Descriptor)
+                        {
+                            Filter = filterItem.Filter,
+                            IsReusable = filterItem.IsReusable
+                        });
+                }
+
+                filters = GetFilters(controllerContext, filterItems);
 
                 return new ControllerActionInvokerState(filters, cacheEntry.ActionMethodExecutor);
             }
@@ -71,7 +84,11 @@ public ControllerActionInvokerState GetState(ControllerContext controllerContext
                 staticFilterItems.Add(new FilterItem(actionDescriptor.FilterDescriptors[i]));
             }
 
-            filters = GetFilters(controllerContext, staticFilterItems);
+            // Create a separate collection as we want to hold onto the statically defined filter items
+            // in order to cache them
+            var allFilterItems = new List<FilterItem>(staticFilterItems);
+
+            filters = GetFilters(controllerContext, allFilterItems);
 
             // Cache the filter items based on the following criteria
             // 1. Are created statically (ex: via filter attributes, added to global filter list etc.)
@@ -90,12 +107,8 @@ public ControllerActionInvokerState GetState(ControllerContext controllerContext
             return new ControllerActionInvokerState(filters, cacheEntry.ActionMethodExecutor);
         }
 
-        private IFilterMetadata[] GetFilters(ActionContext actionContext, List<FilterItem> staticFilterItems)
+        private IFilterMetadata[] GetFilters(ActionContext actionContext, List<FilterItem> filterItems)
         {
-            // Create a separate collection as we want to hold onto the statically defined filter items
-            // in order to cache them
-            var filterItems = new List<FilterItem>(staticFilterItems);
-
             // Execute providers
             var context = new FilterProviderContext(actionContext, filterItems);
 

test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/ControllerActionInvokerCacheTest.cs

@@ -63,22 +63,20 @@ public void GetFilters_CachesFilterFromFactory()
                 new[] { new DefaultFilterProvider() });
             var filterDescriptors = controllerContext.ActionDescriptor.FilterDescriptors;
 
-            // Act - 1
+            // Act & Assert
             var filters = controllerActionInvokerCache.GetState(controllerContext).Filters;
-
-            // Assert - 1
             Assert.Equal(2, filters.Length);
-            var request1Filter1 = Assert.IsType<TestFilter>(filters[0]); // Created by factory
+            var cachedFactoryCreatedFilter = Assert.IsType<TestFilter>(filters[0]); // Created by factory
             Assert.Same(staticFilter, filters[1]); // Cached and the same statically created filter instance
 
-            // Act - 2
-            filters = controllerActionInvokerCache.GetState(controllerContext).Filters;
+            for (var i = 0; i < 5; i++)
+            {
+                filters = controllerActionInvokerCache.GetState(controllerContext).Filters;
 
-            // Assert - 2
-            Assert.Collection(
-                filters,
-                f => Assert.Same(request1Filter1, f), // Cached
-                f => Assert.Same(staticFilter, f)); // Cached and the same statically created filter instance
+                var currentFactoryCreatedFilter = filters[0];
+                Assert.Same(currentFactoryCreatedFilter, cachedFactoryCreatedFilter); // Cached
+                Assert.Same(staticFilter, filters[1]); // Cached
+            }
         }
 
         [Fact]
@@ -96,22 +94,18 @@ public void GetFilters_DoesNotCacheFiltersWithIsReusableFalse()
                 new[] { new DefaultFilterProvider() });
             var filterDescriptors = controllerContext.ActionDescriptor.FilterDescriptors;
 
-            // Act - 1
-            var filters = controllerActionInvokerCache.GetState(controllerContext).Filters;
+            // Act & Assert
+            IFilterMetadata previousFactoryCreatedFilter = null;
+            for (var i = 0; i < 5; i++)
+            {
+                var filters = controllerActionInvokerCache.GetState(controllerContext).Filters;
 
-            // Assert - 1
-            Assert.Equal(2, filters.Length);
-            var request1Filter1 = Assert.IsType<TestFilter>(filters[0]); // Created by factory
-            Assert.Same(staticFilter, filters[1]); // Cached and the same statically created filter instance
+                var currentFactoryCreatedFilter = filters[0];
+                Assert.NotSame(currentFactoryCreatedFilter, previousFactoryCreatedFilter); // Never Cached
+                Assert.Same(staticFilter, filters[1]); // Cached
 
-            // Act - 2
-            filters = controllerActionInvokerCache.GetState(controllerContext).Filters;
-
-            // Assert - 2
-            Assert.Collection(
-                filters,
-                f => Assert.NotSame(request1Filter1, f), // Created by factory again
-                f => Assert.Same(staticFilter, f)); // Cached and the same statically created filter instance
+                previousFactoryCreatedFilter = currentFactoryCreatedFilter;
+            }
         }
 
         [Fact]