So I ran into an interesting behavior today as I deployed my first MVC 4 app tonight. I have a list form that has a filter drop down that allows selection of categories. This list is static and rarely changes so rather than loading these items from the database each time I load the items once and then cache the actual SelectListItem[] array in a static property. However, when we put the site online tonight we immediately noticed that the drop down list was coming up with pre-set values that randomly changed. Didn't take me long to trace this back to the cached list of SelectListItem[]. Clearly the list was getting updated - apparently through the model binding process in the selection postback. To clarify the scenario here's the drop down list definition in the Razor View:@Html.DropDownListFor(mod => mod.QueryParameters.Category, Model.CategoryList, "All Categories")
where Model.CategoryList gets set with:[HttpPost]
[CompressContent]
public ActionResult List(MessageListViewModel model)
{
InitializeViewModel(model);
busEntry entryBus = new busEntry();
var entries = entryBus.GetEntryList(model.QueryParameters);
model.Entries = entries;
model.DisplayMode = ApplicationDisplayModes.Standard;
model.CategoryList = AppUtils.GetCachedCategoryList();
return View(model);
}
The AppUtils.GetCachedCategoryList() method gets the cached list or loads the list on the first access. The code to load up the list is housed in a Web utility class. The method looks like this:/// <summary>
/// Returns a static category list that is cached
/// </summary>
/// <returns></returns>
public static SelectListItem[] GetCachedCategoryList()
{
if (_CategoryList != null)
return _CategoryList;
lock (_SyncLock)
{
if (_CategoryList != null)
return _CategoryList;
var catBus = new busCategory();
var categories = catBus.GetCategories().ToList();
// Turn list into a SelectItem list
var catList= categories
.Select(cat => new SelectListItem() { Text = cat.Name, Value = cat.Id.ToString() })
.ToList();
catList.Insert(0, new SelectListItem()
{
Value = ((int)SpecialCategories.AllCategoriesButRealEstate).ToString(),
Text = "All Categories except Real Estate"
});
catList.Insert(1, new SelectListItem()
{
Value = "-1",
Text = "--------------------------------"
});
_CategoryList = catList.ToArray();
}
return _CategoryList;
}
private static SelectListItem[] _CategoryList ;
This seemed normal enough to me - I've been doing stuff like this forever caching smallish lists in memory to avoid an extra trip to the database. This list is used in various places throughout the application - for the list display and also when adding new items and setting up for notifications etc..
Watch that ModelBinder!
However, it turns out that this code is clearly causing a problem. It appears that the model binder on the [HttpPost] method is actually updating the list that's bound to and changing the actual entry item in the list and setting its selected value. If you look at the code above I'm not setting the SelectListItem.Selected value anywhere - the only place this value can get set is through ModelBinding. Sure enough when stepping through the code I see that when an item is selected the actual model - model.CategoryList[x].Selected - reflects that.
This is bad on several levels: First it's obviously affecting the application behavior - nobody wants to see their drop down list values jump all over the place randomly. But it's also a problem because the array is getting updated by multiple ASP.NET threads which likely would lead to odd crashes from time to time. Not good!
In retrospect the modelbinding behavior makes perfect sense. The actual items and the Selected property is the ModelBinder's way of keeping track of one or more selected values. So while I assumed the list to be read-only, the ModelBinder is actually updating it on a post back producing the rather surprising results. Totally missed this during testing and is another one of those little - "Did you know?" moments.
So, is there a way around this? Yes but it's maybe not quite obvious. I can't change the behavior of the ModelBinder, but I can certainly change the way that the list is generated. Rather than returning the cached list, I can return a brand new cloned list from the cached items like this:/// <summary>
/// Returns a static category list that is cached
/// </summary>
/// <returns></returns>
public static SelectListItem[] GetCachedCategoryList()
{
if (_CategoryList != null)
{
// Have to create new instances via projection
// to avoid ModelBinding updates to affect this
// globally
return _CategoryList
.Select(cat => new SelectListItem()
{
Value = cat.Value,
Text = cat.Text
})
.ToArray();
}
…}
The key is that newly created instances of SelectListItems are returned not just filtered instances of the original list. The key here is 'new instances' so that the ModelBinding updates do not update the actual static instance. The code above uses LINQ and a projection into new SelectListItem instances to create this array of fresh instances. And this code works correctly - no more cross-talk between users.
Unfortunately this code is also less efficient - it has to reselect the items and uses extra memory for the new array. Knowing what I know now I probably would have not cached the list and just take the hit to read from the database. If there is even a possibility of thread clashes I'm very wary of creating code like this. But since the method already exists and handles this load in one place this fix was easy enough to put in.
Live and learn. It's little things like this that can cause some interesting head scratchers sometimes…© Rick Strahl, West Wind Technologies, 2005-2012Posted in MVC ASP.NET .NET
Tweet
!function(d,s,id){var js,fjs=d.getElementsByTagName(s)[0];if(!d.getElementById(id)){js=d.createElement(s);js.id=id;js.src="//platform.twitter.com/widgets.js";fjs.parentNode.insertBefore(js,fjs);}}(document,"script","twitter-wjs");
(function() {
var po = document.createElement('script'); po.type = 'text/javascript'; po.async = true;
po.src = 'https://apis.google.com/js/plusone.js';
var s = document.getElementsByTagName('script')[0]; s.parentNode.insertBefore(po, s);
})();
