mccalltd / AttributeRouting

Define your routes using attributes on actions in ASP.NET MVC and Web API.
http://mccalltd.github.io/AttributeRouting/
MIT License
416 stars 89 forks source link

Collection was modified exceptions. #214

Closed Bitsonthefloor closed 11 years ago

Bitsonthefloor commented 11 years ago

I just implemented AttributeRouting on a fairly busy site and I am now getting a lot of Collection was modified exceptions, which all point to your AttributeRoute.Framework.AttributeRouteVisitor.GetVirtualPath method. It is violating thread safe rules by modifying the route's Constraint dictionary without any locking. While trying to wrap my head around how to make it thread safe I had a thought, why are you removing all IQueryStringRouteConstraints from the route during GetVirtualPath? If you want to not have them match when generating a url, you instead just need to have them return true when RouteDirection.UrlGeneration; this would be the same as removing them, (a true would not cause a match, just not prevent one) but not cause threading issues.

mccalltd commented 11 years ago

This is a big problem. Thanks for reporting. I am looking into the MS source to find the exact spot that made me do this swapping in and out. But from what I recall, if there was a constaint on a param, it was expected to be in the path, not the query. So those query constraints would cause url gen to bonk, as the values weren't getting added to the querystring in the generated path.

Sorry if my description is garbled.... Getting more details now, and will have this fixed today.

mccalltd commented 11 years ago

See:

These are both called from GetVirtualPath and are responsible for the conflict with query string route constraints. Still seeing what I can do regarding thread safety, ie: if I can somehow get rid of the hack I have in there. Worst case I'll resort to a lock.

FYI: Here's the section in System.Web.Http.Routing.HttpParsedRoute that inspired the hack:

// Process constraints keys
if (constraints != null)
{
    // If there are any constraints, mark all the keys as being used so that we don't
    // generate query string items for custom constraints that don't appear as parameters
    // in the URI format.
    foreach (var constraintsItem in constraints)
    {
        unusedNewValues.Remove(constraintsItem.Key);
    }
}

That big comment explains the purpose of the hack. If those query constraints are left in place, then the route values for the query are not included in url gen because the url template does not include params for query constraints.

Bitsonthefloor commented 11 years ago

I don't feel locking would be practical, but the only other solution I can think of would require a change to your framework. Instead of having AttributeRoute inherit from Route have it inherit from BaseRoute, and have a local property that is the Route, then your AttributeRouteVisitor's GetVirtualPath would have to be modified to create a ThreadLocal copy if there are query params, then GetVirtualPath could be called on the thread local or "global" copy. This would prevent much locking, but would definitely change exposed interfaces.

mccalltd commented 11 years ago

Almost finished fixing the problem. No locking. :)

mccalltd commented 11 years ago

Fix pushed to nuget: v3.5.2.

Much thanks for reporting the issue!