maartenba / MvcSiteMapProvider

An ASP.NET MVC SiteMapProvider implementation for the ASP.NET MVC framework.
Microsoft Public License
537 stars 220 forks source link

Concurrency issue creates race condition connected to multiple instances of the same web page #437

Closed inghak closed 8 years ago

inghak commented 8 years ago

Sometimes we get a race condition when the same page is loaded in multible tabs in the browser (Internet Explorer 11).

The error is: System.InvalidOperationException: Collection was modified; enumeration operation may not execute.

The error occures in the following code line: var currentNode = SiteMap.CurrentNode;

While investigating I have found that the error most likely occures while looping over authorizeAttributesin the method VerifyAuthorizeAttributesin the class AuthorizeAttributeAclModule. Probably this collection is modified by another thread while the looping occures. The update from another thread is connected to multiple instances of the page beeing updated simultanously.

The code that probably fails is this: foreach (var authorizeAttribute in authorizeAttributes)

There are several solutions to fix this:

  1. Do a clone/copy of the list before looping
  2. Lock. The problem with lock is that you need to lock on both the read and write side. And we do not control the write side and do not know if it is locked.
  3. Replace the foreach loop with a for loop that investigates the state of the collection and compensates accordingly (reveresed iteration with if-checks / try/catch)

I would suggest option 1. This would simply mean changing to something similar to this: foreach (var authorizeAttribute in authorizeAttributes.ToList()) or:

var cloneOfAuthorizeAttributes = authorizeAttributes.Clone();
foreach (var authorizeAttribute in cloneOfauthorizeAttributes)

The ToList(), ToArray() and similar methods create a copy so that changes in the original collection will not affect the one we are iterating.

Stack trace: System.InvalidOperationException: Collection was modified; enumeration operation may not execute. at System.Collections.Generic.Dictionary2.Enumerator.MoveNext() at System.Collections.Generic.Dictionary2..ctor(IDictionary2 dictionary, IEqualityComparer1 comparer) at System.Web.Routing.RouteValueDictionary..ctor(IDictionary2 dictionary) at MvcSiteMapProvider.DefaultSiteMapNodeUrlResolver.ResolveUrl(MvcSiteMapNode mvcSiteMapNode, String area, String controller, String action, IDictionary2 routeValues) at MvcSiteMapProvider.MvcSiteMapNode.get_Url() at MvcSiteMapProvider.AuthorizeAttributeAclModule.IsAccessibleToUser(IControllerTypeResolver controllerTypeResolver, DefaultSiteMapProvider provider, HttpContext context, SiteMapNode node) at MvcSiteMapProvider.DefaultAclModule.IsAccessibleToUser(IControllerTypeResolver controllerTypeResolver, DefaultSiteMapProvider provider, HttpContext context, SiteMapNode node) at MvcSiteMapProvider.DefaultSiteMapProvider.IsAccessibleToUser(HttpContext context, SiteMapNode node) at System.Web.StaticSiteMapProvider.GetChildNodes(SiteMapNode node) at MvcSiteMapProvider.DefaultSiteMapProvider.FindControllerActionNode(SiteMapNode rootNode, IDictionary2 values, RouteBase route) at MvcSiteMapProvider.DefaultSiteMapProvider.FindControllerActionNode(SiteMapNode rootNode, IDictionary2 values, RouteBase route) at MvcSiteMapProvider.DefaultSiteMapProvider.FindSiteMapNode(HttpContext context, RouteData routeData) at MvcSiteMapProvider.DefaultSiteMapProvider.FindSiteMapNode(HttpContext context) at MvcSiteMapProvider.DefaultSiteMapProvider.get_CurrentNode() at Spv.Archive.Web.Helpers.BreadcrumbHelper.Breadcrumb(HtmlHelper helper) in d:\Builds\54\SPV\Archive.Web-Release-Autsign20140306\Sources\Source\Spv.Archive.Web\Helpers\BreadcrumbHelper.cs:line 13 at ASP._Page_Views_SharedLayout_cshtml.Execute() in c:\inetpub\webapp\Spv.Archive.Web\Views\Shared_Layout.cshtml:line 50 at System.Web.WebPages.WebPageBase.ExecutePageHierarchy() at System.Web.Mvc.WebViewPage.ExecutePageHierarchy() at System.Web.WebPages.WebPageBase.ExecutePageHierarchy(WebPageContext pageContext, TextWriter writer, WebPageRenderingBase startPage) at System.Web.WebPages.WebPageBase.RenderSurrounding(String partialViewName, Action1 body) at System.Web.WebPages.WebPageBase.PopContext() at System.Web.Mvc.ViewResultBase.ExecuteResult(ControllerContext context) at System.Web.Mvc.ControllerActionInvoker.InvokeActionResultFilterRecursive(IList1 filters, Int32 filterIndex, ResultExecutingContext preContext, ControllerContext controllerContext, ActionResult actionResult) at System.Web.Mvc.ControllerActionInvoker.InvokeActionResultFilterRecursive(IList1 filters, Int32 filterIndex, ResultExecutingContext preContext, ControllerContext controllerContext, ActionResult actionResult) at System.Web.Mvc.ControllerActionInvoker.InvokeActionResultWithFilters(ControllerContext controllerContext, IList1 filters, ActionResult actionResult) at System.Web.Mvc.Async.AsyncControllerActionInvoker.<>cDisplayClass1e.b1b(IAsyncResult asyncResult) at System.Web.Mvc.Controller.b1d(IAsyncResult asyncResult, ExecuteCoreState innerState) at System.Web.Mvc.Async.AsyncResultWrapper.WrappedAsyncVoid1.CallEndDelegate(IAsyncResult asyncResult) at System.Web.Mvc.Controller.EndExecuteCore(IAsyncResult asyncResult) at System.Web.Mvc.Async.AsyncResultWrapper.WrappedAsyncVoid1.CallEndDelegate(IAsyncResult asyncResult) at System.Web.Mvc.MvcHandler.b__4(IAsyncResult asyncResult, ProcessRequestState innerState) at System.Web.Mvc.Async.AsyncResultWrapper.WrappedAsyncVoid`1.CallEndDelegate(IAsyncResult asyncResult) at System.Web.HttpApplication.CallHandlerExecutionStep.System.Web.HttpApplication.IExecutionStep.Execute() at System.Web.HttpApplication.ExecuteStep(IExecutionStep step, Boolean& completedSynchronously)

NightOwl888 commented 8 years ago

I don't agree with your assessment of the issue. The stack trace clearly indicates that it is the URL resolver that is throwing the error.

Apparently, your application is dynamically changing the RouteValueDictionary during the middle of the request (and in fact during the middle of resolving the URL through the MVC UrlHelper). A typical application will use routes (built in or custom) to create the route values, which then remain static for the duration of the request.

However, before we jump to too many conclusions, let's start by creating a reproducible demo of the error. Could you build a demo project and either post it on GitHub or zip it and make it available for download?

NightOwl888 commented 8 years ago

I just realized something else based on your stack trace. You are not using MvcSiteMapProvider 4.x, you are using a prior version.

There are lots of known threading/cache issues that were fixed in 4.x, so I strongly recommend you upgrade. It is unlikely you will be able to resolve this issue unless you do.

Upon analyzing the latest source of SiteMapNodeUrlResolver, I realized this issue cannot happen in the latest version because it is using a clone of the route values.

NightOwl888 commented 8 years ago

Closing as this is a known issue with a prior version of MvcSiteMapProvider that is no longer supported.