[SiteMapTitle] Action Filter Attribute Seems to be broken

[pradeepn]

Hi,

[SiteMapTitle] Action Filter Attribute Seems to be broken. Not updating the values from the action result. Only taking the xml title. MvcSiteMapProvider_CacheDuration set to 0.

[SiteMapTitle("Title")]        
public ActionResult Index()
{
    ViewBag.Title = "test";
     return View();
}

Tested with 4.3.0 & 4.4.4.

Thanks.

[NightOwl888]

The SiteMapTitleAttribute doesn't read from the ViewBag object, only from a custom ViewModel or Model object. To make this work, you will need a Model.

[SiteMapTitle("Title")]        
public ActionResult Index()
{
    var model = new HomeModel { Title = "test" };
    ViewBag.Title = model.Title;
     return View(model);
}

public class HomeModel
{
    public string Title { get; set; }
}

See the following documents for other examples:
https://github.com/maartenba/MvcSiteMapProvider/wiki/Using-Action-Filter-Attributes
https://github.com/maartenba/MvcSiteMapProvider/wiki/Routing-Basics

[pradeepn]

Hi,
Thanks for the quick reply.
I tried both. But no luck.

[SiteMapTitle("Title")]    //tried [SiteMapTitle("PageDetails.PageTitle")]  & [SiteMapTitle("SiteMapTitleAttr")]  
public ActionResult Index()
{
     var vm = VMProvider.GetPageDetailsVM();
     ViewBag.Title = vm.PageDetails.PageTitle; //for html title
     ViewData["SiteMapTitleAttr"] = vm.PageDetails.PageTitle;
     //page title needs to be displayed in SitemapTitle Helper & SiteMapPath Helper
     return View("PageDetails", vm);
}

i am using this for display title in

@Html.MvcSiteMap().SiteMapTitle()
@Html.MvcSiteMap().SiteMapPath()

Html title i am getting the value but page title and breadcrumb i am getting only the xml attribute.

Thanks.

[pradeepn]

Hi,
Another thing i note down is this SiteMapTitle ActionFilter throws Null Reference Exception randomly. I can able to reproduce it in continuous reloads like every 2nd or 3rd time i click F5 i get this null reference exception. If i comment SiteMapTitle attribute its working well and respond to each reloads.

Following is the stack trace

 Object reference not set to an instance of an object.
Description: An unhandled exception occurred during the execution of the current web request. Please review the stack trace for more information about the error and where it originated in the code.
Exception Details: System.NullReferenceException: Object reference not set to an instance of an object.
Source Error:
An unhandled exception was generated during the execution of the current web request. Information regarding the origin and location of the exception can be identified using the exception stack trace below.
Stack Trace:
[NullReferenceException: Object reference not set to an instance of an object.]
   MvcSiteMapProvider.Web.Mvc.Filters.SiteMapTitleAttribute.OnActionExecuted(ActionExecutedContext filterContext) +413
   System.Web.Mvc.Async.<>c__DisplayClass4f.b__49() +111
   System.Web.Mvc.Async.<>c__DisplayClass4f.b__49()  +920551 
   System.Web.Mvc.Async.<>c__DisplayClass4f.b__49()  +920551 
   System.Web.Mvc.Async.<>c__DisplayClass37.b__36(IAsyncResult asyncResult) +15
   System.Web.Mvc.Async.<>c__DisplayClass2a.b__20() +33
   System.Web.Mvc.Async.<>c__DisplayClass25.b__22(IAsyncResult asyncResult)  +921356 
   System.Web.Mvc.<>c__DisplayClass1d.b__18(IAsyncResult asyncResult) +28
   System.Web.Mvc.Async.<>c__DisplayClass4.b__3(IAsyncResult ar) +20
   System.Web.Mvc.Controller.EndExecuteCore(IAsyncResult asyncResult) +67
   System.Web.Mvc.Async.<>c__DisplayClass4.b__3(IAsyncResult ar) +20
   System.Web.Mvc.Controller.EndExecute(IAsyncResult asyncResult) +53
   System.Web.Mvc.<>c__DisplayClass8.b__3(IAsyncResult asyncResult) +42
   System.Web.Mvc.Async.<>c__DisplayClass4.b__3(IAsyncResult ar) +20
   System.Web.Mvc.MvcHandler.EndProcessRequest(IAsyncResult asyncResult) +53
   System.Web.CallHandlerExecutionStep.System.Web.HttpApplication.IExecutionStep.Execute() +453
   System.Web.HttpApplication.ExecuteStep(IExecutionStep step, Boolean& completedSynchronously) +371

[NightOwl888]

I created a simple demo project so I could verify it works (and it does): https://github.com/NightOwl888/MvcSiteMapProvider_236

BTW - the information I gave you previously was not entirely accurate - you don't have to create a model, you can also use the ViewData property. Of course, the title will only be overridden for the specific request that calls your action method - in all other cases it will show what you have configured for Title on the SiteMapNode.

SiteMapTitle is generally only useful if you are using preservedRouteParameters to force several requests to match a specific node (such as when you are using CRUD operations to update data). In this case you would usually want your node title to be different for each database record even though the node is always the same. See the How to make MvcSiteMapProvider Remember a User's Position (and the included download) for a complete example of how this works.

As for the exception, please create a small demo project exhibiting the behavior and either post it on GitHub or zip it and make it available for download so I can try to determine what is happening and, if necessary, create a patch.

[pradeepn]

Hi,
Thanks for the sample.
The solution you attached worked perfectly if the cache is not 0;
if i set cache to 0 as below in your sample

<add key="MvcSiteMapProvider_CacheDuration" value="0" />

the text "this is the title" changed to about.
Will this SiteMapTitle attribute will only work with cache?

Thanks.

[pradeepn]

i can also reproduce the Object reference not set to an instance of an object issue in your solution when cache set to 0;
just keep refreshing it. randomnly it throws error. Stacktrace is in previous comment.

[pradeepn]

Hi,
I finally i understand the use of preservedRouteParameters and the use of cache.
Implemented it and it is working as expected. My whole site runs in a specific route parameter and i am using mvc sitemap provider without cache and without any preserved route parameter. As soon as i specified the preservedRouteParameter the everything worked out.

Thanks much!!!

[NightOwl888]

I am unable to reproduce the null reference exception (using the demo project I created, refreshing in rapid succession on the about page), but I have confirmed that the SiteMapTitle attribute doesn't work in this case.

Just out of curiosity, why are you setting CacheDuration to 0? I am not entirely sure that setting a zero cache timeout is a supported configuration for System.Runtime.Caching.MemoryCache, and that might be why you are getting an exception.

If you need to disable caching, it would be far better to inject an ICacheProvider implementation that follows the null object pattern - that is, a cache provider that doesn't store or retrieve anything.

[pradeepn]

Hi,
Yes. Before i am not using the preservedroutepareameter so i have to set cache 0 in order to get the route and title values correctly. Otherwise it will be the guid url. Now i removed cache = 0 and using preserved route parameter in the sitemap file. Its working well. No issues.
Thanks.

[NightOwl888]

I now understand why your title was getting lost with CacheDuration set to 0.

Basically, without a cache every call to the SiteMap would result in a new instance of the SiteMap being built. So setting the title in one line and reading it back in the next line would not work, because you would actually be getting a different instance in the second line.

<mvcSiteMapNode title="The default title"/>

MvcSiteMapProvider.SiteMaps.Current.CurrentNode.Title = "My Title";
var title = MvcSiteMapProvider.SiteMaps.Current.CurrentNode.Title;
// Result of title: The default title

This would normally not be a problem, because when overriding the title this way it is being written to the request cache anyway. However, as an extra precaution to ensure that a different instance with the exact same values doesn't accidentally read back the request cache of the current node, an instance ID (random GUID) is added to the request cache key that can only be read back by the same object instance.

For MvcSiteMapProvider to function correctly without a cache, it would need the SiteMap object to be a request cached so for the scope of an individual request all of the code would be accessing the same SiteMap instance and therefore refer to the same values. Otherwise the SiteMap would be rebuilt several times during the request depending on how many HTML helpers are defined in the markup and how many action filter attributes are defined, etc.

This is a bug. But it is a bug that would be nearly impossible to encounter when using caching - you would literally have to start the request just before the timeout of the cache, write some values into the related request cache before the expiration and read them back after the cache has been rebuilt.

That said, it is still something that should be fixed since there is value in disabling caching in the case where you are using ISiteMapNodeProvider or IDynamicNodeProvider and it is requesting data from a source that is already cached. So when I get a chance, I will add request caching of the SiteMap (in the SiteMapLoader) and the ability to actually disable caching via configuration (using a NullCacheProvider).

[NightOwl888]

@maartenba

I managed to work out what is causing the exception - it is a timing issue. When the cache times out, it immediately calls the SiteMap.Clear() method whether or not any requests are still using the SiteMap instance. This could cause any number of exceptions, the most common being null reference exception.

This issue only happens when cache expires in the middle of a request. Setting the cache timeout to 0 makes the issue much more likely to happen, but requires the request to last at least a few milliseconds in order to get the exception.

Anyway, I worked out how this can be solved, and I have a working prototype. It consists of the following.

1. RefernceConter - keeps track of the number of requests that is currently accessing the SiteMap.
2. SiteMapSpooler - request caches the SiteMap object and controls when to call the reference counter.
3. A SiteMap.MarkForDesruction() method that is used to defer the call to Clear() until all references to the SiteMap are finished with it.

The only problem is that the reference counting must always be decremented when the request is finished with it. My prototype uses a global action filter attribute to dereference the request, but it fails if there is an exception. This means there will be a resource leak if any unhandled exception occurs in any request because the reference count will never reach 0 in that case.

So, there are really only 2 other options (that I know of) than using an MVC action filter attribute:

1. Create an HTTP Module to handle decrementing the reference count even if there is an exception.
2. Make ISiteMap implement IDisposable and always use a using block when accessing it, and modify the reference counting and request cache keys so it no longer matters if the same instance is being accessed for all of the features to work.

While implementing an HTTP module would be easier to upgrade to, HTTP modules are particularly unfriendly for DI because they require a public constructor to function. Not to mention, creating one just to keep track of when it is safe to dispose something feels a bit hacky. There is also the risk that it might not play nice with other HTTP modules.

On the other hand, changing the SiteMap object so there will be a memory leak if it is not used in a using block will cause memory leaks for anyone who doesn't change their code to call it properly. That said, this is the same for using entity framework context - there is a resource leak unless you ensure it is disposed properly. In addition, the amount of code out there that calls the SiteMap object through use of the static method is likely to be small by comparison to those who are using the HTML helpers. Despite the bigger risk of accidentally causing a memory leak, the fact that this method is commonplace in the .NET framework makes me think this is a better way to go.

If we go with the second option, I guess the question is - is this too big of a change for a minor release? I mean, it will require everyone who is accessing the MvcSiteMapProvider.SiteMaps.Current or MvcSiteMapProvider.SiteMaps.GetSiteMap() methods to put them in a using block like this:

using (var siteMap = MvcSiteMapProvider.SiteMaps.Current)
{
    var currentNode = siteMap.CurrentNode;
    // Do something with the node here
}

If it is too big of a change for 4.x, then I guess another option would be to slide in an HTTP module now, and switch to the using block syntax for 5.x.

The thing is, any existing code will still function, it will simply cause a memory leak if it is not changed. This seems to be a "backward compatible" change, although a warning or two about this in the documentation would definitely be in order.

Thoughts?

[maartenba]

To be honest, I would vouch for the IDisposable option. Reference counting should work too, but it has more potential issues (e.g. never refreshing sitemaps because a reference isn't cleared, failure to load the IHttpModule because of server config and so on.

The IDisposable pattern seems good to work with: it creates a temporary lock for those who need it but doesn't interfere with other workflows we currently support.

[NightOwl888]

Actually, the reference counting would still be required because the SiteMap is a shared object. The primary difference between the 2 methods is that the Dispose() method would be decrementing the reference counter rather than relying on an outside component (HTTP module) to do it. Putting it in a using block ensures the reference count will always be decremented no matter what the outcome. However, skipping the call to Dispose() even once will cause the count to be off and the SiteMap (which has circular object references) will never get GC'd.

We still need to wait until all requests are done with the SiteMap before the Clear() method can safely be called, so (unless I have missed another option) the reference counter is essential.

However, since doing it that way means that we won't necessarily have the same SiteMap instance from one using block to the next (even in the same request), the request cache keys need to be adjusted so they contain the SiteMapCacheKey (and in the case of nodes, the Key of the node) instead of an instance id - this will ensure the ID won't change from one object instance to the next and the request cached values will still be accessible.

[maartenba]

My initial idea was to go with a clone-per-request but that would bump the memory footprint required quite a lot. I guess the reference counting is the only way to go, no? On highly loaded sites, this would introduce a chance a sitemap never gets GC'd?

[NightOwl888]

After the reference counting is in place, the amount of traffic shouldn't matter anymore. In our current version, a highly loaded site with a short cache timeout will increase the chance of a null reference exception to the point where it is almost certain.

It would work something like this:

1. The SiteMap object would be cached, holding a pointer to it.
2. Each using block would request-cache the SiteMap (in a dictionary in case more than one is accessed), holding a pointer to it, and incrementing the reference count by 1.
3. When the SiteMap cache times out, it will be evicted from the cache immediately (removing the pointer) and will fire off the event to call MarkForDestruction() (which sets a Boolean variable in the SiteMap object). MarkForDestruction() will call Clear() if the reference count is 0.
4. At the end of any using block, Dispose() will be called, decrementing the reference count by 1 and removing the SiteMap pointer from the request cache. When the reference count reaches 0, Clear() will be called if the MarkForDestruction() method had been called previously.
5. The next using block (potentially within the same request) will rebuild and cache the SiteMap. Any values that are request cached for the SiteMap or a specific node will be accessed using the same key, so they will still be available even though we have a new SiteMap instance.

The only chance of a memory leak is the case where the using block is not used, and then it is certain to happen. The problem is - any code out there that is accessing the SiteMap statically is certainly not fenced in a using block because it is not currently supported.

So, that is why I am hesitant to throw it out there in version 4.x - this is a major API change for those (likely few) developers who are using it. Any usage of the HTML helpers would be covered because we can easily change the calls to the static SiteMaps.Current method to the using block.

On the other hand, using a HTTP module doesn't change the API so it would not require any code changes. I suppose it could be registered programmatically, but I think even that will require an HTTP module to be registered in the web.config file to fire it off (unless it will work with WebActivator's PreApplicationStart method).

Still, when using an HTTP module, it will likely mean switching to a strategy that uses a service locator for DI otherwise the SiteMapLoader, SiteMapSpooler, etc. won't be able to be swapped out. I'd really prefer not to go down that road even if it means an API change that could potentially cause a memory leak.

So, I am torn between

1. Creating what amounts to an ugly hack to fix an almost unnoticeable problem in order to comply with semantic versioning and fix this in 4.x
2. Fixing this the right way, but breaking semantic versioning
3. Fixing this the right way, but waiting until 5.x to fix it to comply with semantic versioning
4. Fixing this the right way, but waiting until 5.x to fix it, and putting in the ugly hack to fix 4.x and comply with semantic versioning

[maartenba]

Let's go with 3 then? We can do the MVC5 support in v5 as well. Gives us some time to test.

[NightOwl888]

Agreed. 3 seems like the best choice, but means it might be a while before the exceptions are fixed. But then, nobody is complaining about them - most likely because when they are encountered they are impossible to duplicate.

There are some other design issues that I have been keeping track of in my head (that I should have written down) that will need to be addressed in v5. For now, I will document them here, but eventually they will need to be made into their own separate issues (perhaps labeled with a v5 label to keep them separate). I will also keep adding to this list as I think of them so they are listed in one place.

1. The ISiteMapBuilder interface should not be allowed to support multiplicity because this allows a way to circumvent the node ordering logic (which is critical for ensuring that a node will be able to reference its parent node).
2. The BuildSiteMap() method should be eliminated, favoring building the SiteMap using an abstract factory. This will prevent any user from inadvertently calling it manually, which isn't allowed. See this answer for why this should not be public.
3. The SiteMapCacheKey should be changed to SiteMapName. ISiteMapCacheKeyGenerator should be changed to ISiteMapNameProvider to be consistent with the rest of the framework.
4. The SiteMapCacheKey should be set in a local read-only property of the SiteMap.
5. The SiteMapCacheKey should be used instead of an instanceId when doing request caching of SiteMap and SiteMapNode fields.
6. The SiteMap's internal dictionaries should be changed to using a key based on the unique routing signature of the node, rather than based on URL to determine uniqueness. This is primarily because resolving URLs based on route values cannot always be done at the time the SiteMap is constructed. Those nodes based on explicit URLs should continue to use a URL based key (in a separate dictionary).
7. The API of the UrlResolver should be modified to facilitate resolving both a route-based URL and resolving a relative URL based URL in a way that allows these pieces to be composed independently.
8. The old SiteMap builders, SiteMapPreserveRouteDataAttribute, and other obsolete members should be removed from the project.
9. The logic for defaulting the Description property to the value of the Title property should be moved from the ISiteMapNodeProvider's to the HTML helpers so it can be more easily changed by end users (as described in If Sitemap description is left empty it takes the value of Sitemap title attribute #238).
10. The concept of visibilityAffectsDescendants should be revisited. Currently, the logic is inconsistent between the HTML helpers and the /sitemap.xml endpoint. Either this needs to be a SiteMap wide setting, or it should be eliminated altogether as an option, since visibility that affects descendant nodes is confusing at best.
11. Implicit localization should either be removed entirely, or there needs to be some way to facilitate localizing data based on ISiteMapDataProvider implementation rather than per ISiteMap implementation. Logically it makes sense to keep localized data in the same data store as the data being localized for this to work right, so it should be possible to create a provider for each data store (as described in Implicit Localization not working #228).
12. The node key logic should be dealt with at the point in time where the node is added to the SiteMap (rather than in ISiteMapNodeProvider) so its values can never be out of sync with the SiteMapNode values, including when node values are inherited from the parent node.
13. The ISiteMapNodeKeyGenerator should be changed to ISiteMapNodeKeyProvider to be consistent with the rest of the framework. There needs to be separate implementations for URL key vs RouteValue key. The default implementations should be the same "URL" key or "RouteValue" key that is generated for the internal SiteMap dictionaries.
14. The node inheritance logic needs to be consolidated and moved into a service that can be replaced.
15. Feature: XML inheritance should be extended to apply to any attribute including custom attributes.
16. Logic for ignoring RouteValues and Attributes should be moved into a centralized location instead of being done directly in ISiteMapNodeProvider implementations.
17. Logic for transferring collections and dictionaries should be moved into a centralized location instead of being done directly in ISiteMapNodeProvider implementations.
18. Feature: There should be a way to dynamically add nodes to the SiteMap per request. Further, there should be a way to cache these nodes on a per-session basis. My thought is to add a read-only property to all dynamic node sources called "cacheLevel" or something similar that can be set to "shared", "request", or "session". The node providers will be run per request and only instantiate the "request" nodes and add them to the SiteMap. The node providers will also be run when a new session is started to instantiate the "session" nodes and add them to the SiteMap. The SiteMap's internal dictionaries will need to be made into specialized "split" dictionaries that store the request level nodes in a separate location that is cleaned up when the request is complete. The session level nodes will likely be added by a decorator class that adds them per request from a session cache so it can take advantage of the same request-level mechanism. This will facilitate Caching per-user #16 in any way it is not covered currently.
19. The default setup of the ISiteMapCacheKeyToBuilderSetMapper should be changed to a 1 to 1 mapping (or at least there should be a built-in implementation that maps 1 to 1).
20. Throw an exception, rather than make a GUID URL, when the route value portion of SiteMapNodeUrlResolver can't resolve the URL. The SiteMapNodeUrlResolver should not be called during building the SiteMap to set the URL key. In other cases, this call can be disabled (and the exception message should explain how to do this). See The virtual path maps to another application #235. We decided to instead return a "#".
21. The ISiteMap and ISiteMapNode interfaces should be divided into 2 interfaces each to hide the members that are not allowed from the presentation layer.
22. Feature: There should be a built-in way to cache the sitemap to disk by changing a configuration setting.
23. Feature: There should be a way to stream the model for /sitemap.xml to disk so very large sitemaps perform adequately. My thought is to make an extremely long timeout like 12 hours and only recreate it when the timeout expires. That will give search engines a chance to reuse the generated model from a previous request so it doesn't have to recreate it every time, especially when an index sitemap page is used.
24. There should be a way to cache the nodes in chunks (or perhaps even individually) so performance of very large SiteMaps with hundreds of thousands of nodes can be managed better.
25. The HTML helpers and their models need to be refactored so their implementations can be swapped with alternate implementations via DI.
26. Feature: There should be a RouteBase implementation that 301 redirects to the correct letter casing of the URL from any invalid letter casing of the same URL. This feature should support using a custom view for the redirect (in case the browser doesn't respect the 301 status code).

Many of these items depend on the fact that the key logic needs to change, which makes them ideal to implement together instead of as one-offs.

v5 is clearly going to need a serious commitment of time, which is something I can't provide right now. I don't think MVC5 support can wait for all of these to get completed and facilitating it should be an additive change that doesn't affect backward compatibility of the current API.

I also think that item 4 and 5 above can probably be done in v4 so at least the original problem described in this issue can be addressed now without changing the API too much (only the constructor of the SiteMap will be changed, which isn't likely an object many people have subclassed or implemented themselves).

[NightOwl888]

Both issues:

1. The SiteMapTitle not functioning when cache duration is 0.
2. The random exceptions being thrown when the cache times out.

Have been addressed, so I am marking this closed.