Aspnetcore: IUrlHelper provided by IUrlHelperFactory is not thread safe and throws exceptions

[lostllama]

Describe the bug

If IUrlHelper is used in a multi-threaded async request where URLs end up being generated concurrently, exceptions such as this get thrown:

System.ArgumentException: 'An element with the key 'id' already exists in the RouteValueDictionary.
Arg_ParamName_Name'

or

System.ArgumentNullException: Value cannot be null. Parameter name: key

To Reproduce

Steps to reproduce the behavior:

1. Create a new controller like so:

    [Route("api/[controller]")]
    [ApiController]
    public class DummyController : ControllerBase
    {
        [HttpGet]
        [Route("targetMethod", Name = "myroute")]
        [AllowAnonymous]
        public IActionResult TestMethod(Guid id)
        {
            return Ok();
        }

        [HttpGet]
        [Route("demonstration")]
        [AllowAnonymous]
        public IActionResult Demonstration([FromServices]IUrlHelperFactory helperFactory, [FromServices]IActionContextAccessor contextAccessor)
        {
            var urls = new ConcurrentBag<string>();
            Parallel.For(0, 50, v =>
            {
                var urlHelper = helperFactory.GetUrlHelper(contextAccessor.ActionContext);
                var dictionary = new Dictionary<string, object>
                {
                    { "id", Guid.NewGuid() }
                };
                urls.Add(urlHelper.RouteUrl("myroute", dictionary));
            });
            return Ok();
        }
    }

1. Visit {API_URL}/api/dummy/demonstration.

Expected behavior

URLs should be generated successfully.

Additional context

Microsoft.AspNetCore.All version: 2.2.2

The issue seems to start here and the heart of it is in this method which converts a Dictionary into a RouteValueDictionary.

The UrlHelperBase declares a shared read-only RouteValueDictionaryField which is used in the GetValuesDictionary method (I'm not sure why - it doesn't need to be). Calling this from multiple threads means multiple threads trying to access a single RouteValueDictionaryField object.

As you can see in my example, I'm requesting the factory create a new IUrlHelper for every URL I create. Looking at the code for UrlHelperFactory, it seems that only one instance is created per request and then that instance is returned each time another one is requested.

A simple work around is to pass a RouteValueDictionary instead of a Dictionary, since that will be used as-is, without running it through the GetValuesDictionary method.

[pranavkm]

Unless explicitly documented, none of the types in ASP.NET Core are designed to be thread safe. Calling it in parallel will result in non-deterministic behavior.

[lostllama]

@pranavkm It seems very strange to me that this would be "by design". The GetValuesDictionary method is the only method in UrlHelper that uses the _routeValueDictionary field, and then it returns a reference to this field. If _routeValueDictionary were replaced with a method variable inside the GetValuesDictionary, it would solve this problem and make the RouteUrl method thread safe.

I don't think it's unreasonable to expect a method that generates URLs to controllers to be thread safe with respect to the parameters of the URL we're requesting.

[Porges]

I believe this should be reopened and considered a bug. We generate URIs on lots of different threads in our controllers, and this has worked consistently until we updated to 2.2. It seems the new endpoint router implementation has broken this.

Generating a URI doesn't appear to be something that should mutate hidden state and give nondeterministic results when invoked from multiple threads.

Putting a lock around URI generation (!) is going to nullify any "performance" gained by sharing buffers and value dictionaries here: https://github.com/aspnet/AspNetCore/blob/c565386a3ed135560bc2e9017aa54a950b4e35dd/src/Mvc/Mvc.Core/src/Routing/UrlHelperBase.cs#L17-L21

[lostllama]

@Porges

There is a newer LinkGenerator class in .NET Core nowadays, which _might_ be thread safe, although I haven't tested it myself.