Filters provided by filter providers are made to never be cached

[Fixes #4504] Possible double-execution of filter providers
This commit is contained in:
Kiran Challa 2016-04-21 13:44:24 -07:00
parent 967001f923
commit a259638d79
4 changed files with 253 additions and 233 deletions

View File

@ -45,96 +45,59 @@ namespace Microsoft.AspNetCore.Mvc.Internal
}
}
public Entry GetCacheEntry(ControllerContext controllerContext)
public ControllerActionInvokerState GetState(ControllerContext controllerContext)
{
// Filter instances from statically defined filter descriptors + from filter providers
IFilterMetadata[] filters;
var cache = CurrentCache;
var actionDescriptor = controllerContext.ActionDescriptor;
Entry entry;
if (cache.Entries.TryGetValue(actionDescriptor, out entry))
Entry cacheEntry;
if (cache.Entries.TryGetValue(actionDescriptor, out cacheEntry))
{
return entry;
filters = GetFilters(controllerContext, cacheEntry.FilterItems);
return new ControllerActionInvokerState(filters, cacheEntry.ActionMethodExecutor);
}
var executor = ObjectMethodExecutor.Create(actionDescriptor.MethodInfo, actionDescriptor.ControllerTypeInfo);
var executor = ObjectMethodExecutor.Create(
actionDescriptor.MethodInfo,
actionDescriptor.ControllerTypeInfo);
var items = new List<FilterItem>(actionDescriptor.FilterDescriptors.Count);
var staticFilterItems = new List<FilterItem>(actionDescriptor.FilterDescriptors.Count);
for (var i = 0; i < actionDescriptor.FilterDescriptors.Count; i++)
{
items.Add(new FilterItem(actionDescriptor.FilterDescriptors[i]));
staticFilterItems.Add(new FilterItem(actionDescriptor.FilterDescriptors[i]));
}
ExecuteProviders(controllerContext, items);
filters = GetFilters(controllerContext, staticFilterItems);
var filters = ExtractFilters(items);
var allFiltersCached = true;
for (var i = 0; i < items.Count; i++)
// Cache the filter items based on the following criteria
// 1. Are created statically (ex: via filter attributes, added to global filter list etc.)
// 2. Are re-usable
for (var i = 0; i < staticFilterItems.Count; i++)
{
var item = items[i];
var item = staticFilterItems[i];
if (!item.IsReusable)
{
item.Filter = null;
allFiltersCached = false;
}
}
cacheEntry = new Entry(staticFilterItems, executor);
cache.Entries.TryAdd(actionDescriptor, cacheEntry);
if (allFiltersCached)
{
entry = new Entry(filters, null, executor);
}
else
{
entry = new Entry(null, items, executor);
}
cache.Entries.TryAdd(actionDescriptor, entry);
return entry;
return new ControllerActionInvokerState(filters, cacheEntry.ActionMethodExecutor);
}
public IFilterMetadata[] GetFilters(ControllerContext controllerContext)
private IFilterMetadata[] GetFilters(ActionContext actionContext, List<FilterItem> staticFilterItems)
{
var entry = GetCacheEntry(controllerContext);
return GetFiltersFromEntry(entry, controllerContext);
}
// 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);
public ObjectMethodExecutor GetControllerActionMethodExecutor(ControllerContext controllerContext)
{
var entry = GetCacheEntry(controllerContext);
return entry.ActionMethodExecutor;
}
public IFilterMetadata[] GetFiltersFromEntry(Entry entry, ActionContext actionContext)
{
Debug.Assert(entry.Filters != null || entry.FilterItems != null);
if (entry.Filters != null)
{
return entry.Filters;
}
var items = new List<FilterItem>(entry.FilterItems.Count);
for (var i = 0; i < entry.FilterItems.Count; i++)
{
var item = entry.FilterItems[i];
if (item.IsReusable)
{
items.Add(item);
}
else
{
items.Add(new FilterItem(item.Descriptor));
}
}
ExecuteProviders(actionContext, items);
return ExtractFilters(items);
}
private void ExecuteProviders(ActionContext actionContext, List<FilterItem> items)
{
var context = new FilterProviderContext(actionContext, items);
// Execute providers
var context = new FilterProviderContext(actionContext, filterItems);
for (var i = 0; i < _filterProviders.Length; i++)
{
@ -145,14 +108,12 @@ namespace Microsoft.AspNetCore.Mvc.Internal
{
_filterProviders[i].OnProvidersExecuted(context);
}
}
private IFilterMetadata[] ExtractFilters(List<FilterItem> items)
{
// Extract filter instances from statically defined filters and filter providers
var count = 0;
for (var i = 0; i < items.Count; i++)
for (var i = 0; i < filterItems.Count; i++)
{
if (items[i].Filter != null)
if (filterItems[i].Filter != null)
{
count++;
}
@ -166,9 +127,9 @@ namespace Microsoft.AspNetCore.Mvc.Internal
{
var filters = new IFilterMetadata[count];
var filterIndex = 0;
for (int i = 0; i < items.Count; i++)
for (int i = 0; i < filterItems.Count; i++)
{
var filter = items[i].Filter;
var filter = filterItems[i].Filter;
if (filter != null)
{
filters[filterIndex++] = filter;
@ -186,25 +147,38 @@ namespace Microsoft.AspNetCore.Mvc.Internal
Version = version;
}
public ConcurrentDictionary<ActionDescriptor, Entry> Entries { get; } =
public ConcurrentDictionary<ActionDescriptor, Entry> Entries { get; } =
new ConcurrentDictionary<ActionDescriptor, Entry>();
public int Version { get; }
}
public struct Entry
private struct Entry
{
public Entry(IFilterMetadata[] filters, List<FilterItem> items, ObjectMethodExecutor executor)
public Entry(List<FilterItem> items, ObjectMethodExecutor executor)
{
FilterItems = items;
Filters = filters;
ActionMethodExecutor = executor;
}
public IFilterMetadata[] Filters { get; }
public List<FilterItem> FilterItems { get; }
public ObjectMethodExecutor ActionMethodExecutor { get; }
}
public struct ControllerActionInvokerState
{
public ControllerActionInvokerState(
IFilterMetadata[] filters,
ObjectMethodExecutor actionMethodExecutor)
{
Filters = filters;
ActionMethodExecutor = actionMethodExecutor;
}
public IFilterMetadata[] Filters { get; }
public ObjectMethodExecutor ActionMethodExecutor { get; set; }
}
}
}

View File

@ -123,9 +123,9 @@ namespace Microsoft.AspNetCore.Mvc.Internal
public virtual async Task InvokeAsync()
{
var cacheEntry = _controllerActionInvokerCache.GetCacheEntry(Context);
_filters = _controllerActionInvokerCache.GetFiltersFromEntry(cacheEntry, Context);
_controllerActionMethodExecutor = cacheEntry.ActionMethodExecutor;
var controllerActionInvokerState = _controllerActionInvokerCache.GetState(Context);
_filters = controllerActionInvokerState.Filters;
_controllerActionMethodExecutor = controllerActionInvokerState.ActionMethodExecutor;
_cursor = new FilterCursor(_filters);
Context.ModelState.MaxAllowedErrors = _maxModelValidationErrors;

View File

@ -1,34 +0,0 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
using System;
using System.Collections.Generic;
using Microsoft.AspNetCore.Mvc.Filters;
namespace Microsoft.AspNetCore.Mvc.Internal
{
public class FilterItemOrderComparer : IComparer<FilterItem>
{
private static readonly FilterItemOrderComparer _comparer = new FilterItemOrderComparer();
public static FilterItemOrderComparer Comparer
{
get { return _comparer; }
}
public int Compare(FilterItem x, FilterItem y)
{
if (x == null)
{
throw new ArgumentNullException(nameof(x));
}
if (y == null)
{
throw new ArgumentNullException(nameof(y));
}
return FilterDescriptorOrderComparer.Comparer.Compare(x.Descriptor, y.Descriptor);
}
}
}

View File

@ -6,8 +6,8 @@ using System.Reflection;
using Microsoft.AspNetCore.Http;
using Microsoft.AspNetCore.Mvc.Controllers;
using Microsoft.AspNetCore.Mvc.Filters;
using Microsoft.AspNetCore.Mvc.Infrastructure;
using Microsoft.AspNetCore.Routing;
using Microsoft.Extensions.DependencyInjection;
using Xunit;
namespace Microsoft.AspNetCore.Mvc.Internal
@ -18,198 +18,278 @@ namespace Microsoft.AspNetCore.Mvc.Internal
public void GetFilters_CachesAllFilters()
{
// Arrange
var services = CreateServices();
var cache = CreateCache(new DefaultFilterProvider());
var action = new ControllerActionDescriptor()
{
FilterDescriptors = new[]
var staticFilter1 = new TestFilter();
var staticFilter2 = new TestFilter();
var controllerContext = CreateControllerContext(new[]
{
new FilterDescriptor(new TestFilter(), FilterScope.Action),
new FilterDescriptor(new TestFilter(), FilterScope.Action),
},
MethodInfo = typeof(ControllerActionInvokerCache).GetMethod(
nameof(ControllerActionInvokerCache.GetControllerActionMethodExecutor)),
ControllerTypeInfo = typeof(ControllerActionInvokerCache).GetTypeInfo()
};
var context = new ControllerContext(new ActionContext(
new DefaultHttpContext(),
new RouteData(),
action));
new FilterDescriptor(staticFilter1, FilterScope.Action),
new FilterDescriptor(staticFilter2, FilterScope.Action),
});
var controllerActionInvokerCache = CreateControllerActionInvokerCache(
controllerContext,
new[] { new DefaultFilterProvider() });
// Act - 1
var filters1 = cache.GetFilters(context);
var request1Filters = controllerActionInvokerCache.GetState(controllerContext).Filters;
// Assert - 1
Assert.Collection(
filters1,
f => Assert.Same(action.FilterDescriptors[0].Filter, f), // Copied by provider
f => Assert.Same(action.FilterDescriptors[1].Filter, f)); // Copied by provider
request1Filters,
f => Assert.Same(staticFilter1, f),
f => Assert.Same(staticFilter2, f));
// Act - 2
var filters2 = cache.GetFilters(context);
Assert.Same(filters1, filters2);
var request2Filters = controllerActionInvokerCache.GetState(controllerContext).Filters;
// Assert - 2
Assert.Collection(
filters2,
f => Assert.Same(action.FilterDescriptors[0].Filter, f), // Cached
f => Assert.Same(action.FilterDescriptors[1].Filter, f)); // Cached
request2Filters,
f => Assert.Same(staticFilter1, f),
f => Assert.Same(staticFilter2, f));
}
[Fact]
public void GetFilters_CachesFilterFromFactory()
{
// Arrange
var services = CreateServices();
var cache = CreateCache(new DefaultFilterProvider());
var action = new ControllerActionDescriptor()
{
FilterDescriptors = new[]
var staticFilter = new TestFilter();
var controllerContext = CreateControllerContext(new[]
{
new FilterDescriptor(new TestFilterFactory() { IsReusable = true }, FilterScope.Action),
new FilterDescriptor(new TestFilter(), FilterScope.Action),
},
MethodInfo = typeof(ControllerActionInvokerCache).GetMethod(
nameof(ControllerActionInvokerCache.GetControllerActionMethodExecutor)),
ControllerTypeInfo = typeof(ControllerActionInvokerCache).GetTypeInfo()
};
var context = new ControllerContext(new ActionContext(
new DefaultHttpContext(),
new RouteData(),
action));
new FilterDescriptor(staticFilter, FilterScope.Action),
});
var controllerActionInvokerCache = CreateControllerActionInvokerCache(
controllerContext,
new[] { new DefaultFilterProvider() });
var filterDescriptors = controllerContext.ActionDescriptor.FilterDescriptors;
// Act - 1
var filters1 = cache.GetFilters(context);
var filters = controllerActionInvokerCache.GetState(controllerContext).Filters;
// Assert - 1
Assert.Collection(
filters1,
f => Assert.NotSame(action.FilterDescriptors[0].Filter, f), // Created by factory
f => Assert.Same(action.FilterDescriptors[1].Filter, f)); // Copied by provider
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
// Act - 2
var filters2 = cache.GetFilters(context);
Assert.Same(filters1, filters2);
filters = controllerActionInvokerCache.GetState(controllerContext).Filters;
// Assert - 2
Assert.Collection(
filters2,
f => Assert.Same(filters1[0], f), // Cached
f => Assert.Same(filters1[1], f)); // Cached
filters,
f => Assert.Same(request1Filter1, f), // Cached
f => Assert.Same(staticFilter, f)); // Cached and the same statically created filter instance
}
[Fact]
public void GetFilters_DoesNotCacheFiltersWithIsReusableFalse()
{
// Arrange
var services = CreateServices();
var cache = CreateCache(new DefaultFilterProvider());
var action = new ControllerActionDescriptor()
{
FilterDescriptors = new[]
var staticFilter = new TestFilter();
var controllerContext = CreateControllerContext(new[]
{
new FilterDescriptor(new TestFilterFactory() { IsReusable = false }, FilterScope.Action),
new FilterDescriptor(new TestFilter(), FilterScope.Action),
},
MethodInfo = typeof(ControllerActionInvokerCache).GetMethod(
nameof(ControllerActionInvokerCache.GetControllerActionMethodExecutor)),
ControllerTypeInfo = typeof(ControllerActionInvokerCache).GetTypeInfo()
};
var context = new ControllerContext(new ActionContext(
new DefaultHttpContext(),
new RouteData(),
action));
new FilterDescriptor(staticFilter, FilterScope.Action),
});
var controllerActionInvokerCache = CreateControllerActionInvokerCache(
controllerContext,
new[] { new DefaultFilterProvider() });
var filterDescriptors = controllerContext.ActionDescriptor.FilterDescriptors;
// Act - 1
var filters1 = cache.GetFilters(context);
var filters = controllerActionInvokerCache.GetState(controllerContext).Filters;
// Assert - 1
Assert.Collection(
filters1,
f => Assert.NotSame(action.FilterDescriptors[0].Filter, f), // Created by factory
f => Assert.Same(action.FilterDescriptors[1].Filter, f)); // Copied by provider
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
// Act - 2
var filters2 = cache.GetFilters(context);
Assert.NotSame(filters1, filters2);
filters = controllerActionInvokerCache.GetState(controllerContext).Filters;
// Assert - 2
Assert.Collection(
filters2,
f => Assert.NotSame(filters1[0], f), // Created by factory (again)
f => Assert.Same(filters1[1], f)); // Cached
filters,
f => Assert.NotSame(request1Filter1, f), // Created by factory again
f => Assert.Same(staticFilter, f)); // Cached and the same statically created filter instance
}
[Fact]
public void GetControllerActionMethodExecutor_CachesExecutor()
public void GetControllerActionMethodExecutor_CachesActionMethodExecutor()
{
// Arrange
var services = CreateServices();
var cache = CreateCache(new DefaultFilterProvider());
var filter = new TestFilter();
var controllerContext = CreateControllerContext(new[]
{
new FilterDescriptor(filter, FilterScope.Action)
});
var controllerActionInvokerCache = CreateControllerActionInvokerCache(
controllerContext,
new[] { new DefaultFilterProvider() });
var action = new ControllerActionDescriptor()
{
FilterDescriptors = new[]
// Act
var cacheEntry1 = controllerActionInvokerCache.GetState(controllerContext);
var cacheEntry2 = controllerActionInvokerCache.GetState(controllerContext);
// Assert
Assert.Same(cacheEntry1.ActionMethodExecutor, cacheEntry2.ActionMethodExecutor);
}
[Theory]
[InlineData(true)]
[InlineData(false)]
public void GetFilters_FiltersAddedByFilterProviders_AreNeverCached(bool reusable)
{
// Arrange
var customFilterProvider = new TestFilterProvider(
providerExecuting: (providerContext) =>
{
var filter = new TestFilter(providerContext.ActionContext.HttpContext.Items["name"] as string);
providerContext.Results.Add(
new FilterItem(new FilterDescriptor(filter, FilterScope.Global), filter)
{
IsReusable = reusable
});
},
providerExecuted: null);
var staticFilter = new TestFilter();
var controllerContext = CreateControllerContext(new[]
{
new FilterDescriptor(new TestFilterFactory() { IsReusable = false }, FilterScope.Action),
new FilterDescriptor(new TestFilter(), FilterScope.Action),
},
MethodInfo = typeof(ControllerActionInvokerCache).GetMethod(
nameof(ControllerActionInvokerCache.GetControllerActionMethodExecutor)),
ControllerTypeInfo = typeof(ControllerActionInvokerCache).GetTypeInfo()
};
var context = new ControllerContext(
new ActionContext(new DefaultHttpContext(),
new RouteData(),
action));
new FilterDescriptor(staticFilter, FilterScope.Action),
});
var controllerActionInvokerCache = CreateControllerActionInvokerCache(
controllerContext,
new IFilterProvider[] { new DefaultFilterProvider(), customFilterProvider });
var filterDescriptors = controllerContext.ActionDescriptor.FilterDescriptors;
// Act - 1
var executor1 = cache.GetControllerActionMethodExecutor(context);
controllerContext.HttpContext.Items["name"] = "foo";
var filters = controllerActionInvokerCache.GetState(controllerContext).Filters;
Assert.NotNull(executor1);
var filters1 = cache.GetFilters(context);
Assert.NotNull(filters1);
// Assert - 1
Assert.Equal(3, 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 request1Filter3 = Assert.IsType<TestFilter>(filters[2]); // Created by custom filter provider
Assert.Equal("foo", request1Filter3.Data);
// Act - 2
var executor2 = cache.GetControllerActionMethodExecutor(context);
controllerContext.HttpContext.Items["name"] = "bar";
filters = controllerActionInvokerCache.GetState(controllerContext).Filters;
Assert.Same(executor1, executor2);
// Assert -2
Assert.Equal(3, filters.Length);
var request2Filter1 = Assert.IsType<TestFilter>(filters[0]);
Assert.NotSame(request1Filter1, request2Filter1); // Created by factory
Assert.Same(staticFilter, filters[1]); // Cached and the same statically created filter instance
var request2Filter3 = Assert.IsType<TestFilter>(filters[2]);
Assert.NotSame(request1Filter3, request2Filter3); // Created by custom filter provider again
Assert.Equal("bar", request2Filter3.Data);
}
private class TestFilter : IFilterMetadata
{
public TestFilter()
{
}
public TestFilter(string data)
{
Data = data;
}
public string Data { get; }
}
private class TestFilterFactory : IFilterFactory
{
private TestFilter testFilter;
public bool IsReusable { get; set; }
public IFilterMetadata CreateInstance(IServiceProvider serviceProvider)
{
return new TestFilter();
if (IsReusable)
{
if (testFilter == null)
{
testFilter = new TestFilter();
}
return testFilter;
}
else
{
return new TestFilter();
}
}
}
private static IServiceProvider CreateServices()
private class TestFilterProvider : IFilterProvider
{
return new ServiceCollection().BuildServiceProvider();
private readonly Action<FilterProviderContext> _providerExecuting;
private readonly Action<FilterProviderContext> _providerExecuted;
public TestFilterProvider(
Action<FilterProviderContext> providerExecuting,
Action<FilterProviderContext> providerExecuted,
int order = 0)
{
_providerExecuting = providerExecuting;
_providerExecuted = providerExecuted;
Order = order;
}
public int Order { get; }
public void OnProvidersExecuting(FilterProviderContext context)
{
_providerExecuting?.Invoke(context);
}
public void OnProvidersExecuted(FilterProviderContext context)
{
_providerExecuted?.Invoke(context);
}
}
private static ControllerActionInvokerCache CreateCache(params IFilterProvider[] providers)
private class TestController
{
var services = CreateServices();
var descriptorProvider = new ActionDescriptorCollectionProvider(services);
return new ControllerActionInvokerCache(descriptorProvider, providers);
public void Index()
{
}
}
private class CustomActionDescriptorCollectionProvider : IActionDescriptorCollectionProvider
{
public CustomActionDescriptorCollectionProvider(ControllerActionDescriptor[] actionDescriptors)
{
ActionDescriptors = new ActionDescriptorCollection(actionDescriptors, version: 1);
}
public ActionDescriptorCollection ActionDescriptors { get; }
}
private static ControllerActionInvokerCache CreateControllerActionInvokerCache(
ControllerContext controllerContext,
IFilterProvider[] filterProviders)
{
var descriptorProvider = new CustomActionDescriptorCollectionProvider(
new[] { controllerContext.ActionDescriptor });
return new ControllerActionInvokerCache(descriptorProvider, filterProviders);
}
private static ControllerContext CreateControllerContext(FilterDescriptor[] filterDescriptors)
{
var actionDescriptor = new ControllerActionDescriptor()
{
FilterDescriptors = filterDescriptors,
MethodInfo = typeof(TestController).GetMethod(nameof(TestController.Index)),
ControllerTypeInfo = typeof(TestController).GetTypeInfo()
};
var actionContext = new ActionContext(new DefaultHttpContext(), new RouteData(), actionDescriptor);
return new ControllerContext(actionContext);
}
}
}