From 7db6395d5d46dc7ce774132c8e7d09d66fcfbcc1 Mon Sep 17 00:00:00 2001
From: Ryan Nowak <rynowak@microsoft.com>
Date: Fri, 15 Nov 2019 09:37:46 -0800
Subject: [PATCH] Make constraint cache thread safe

Fixes: #17101

This changes the constraint cache to use ConcurrentDictionary. This code
is invoked in a multithreaded way in Blazor server resulting in internal
failures in dictionary.

Since this is a threading issue there's no good way to unit test it, but
I noticed we're missing tests in general for this class, so I added a
few for the caching behavior.
---
 .../Components/src/Routing/RouteConstraint.cs | 13 ++++---
 .../test/Routing/RouteConstraintTest.cs       | 36 +++++++++++++++++++
 2 files changed, 45 insertions(+), 4 deletions(-)
 create mode 100644 src/Components/Components/test/Routing/RouteConstraintTest.cs

diff --git a/src/Components/Components/src/Routing/RouteConstraint.cs b/src/Components/Components/src/Routing/RouteConstraint.cs
index 766f6a2faed6..ca7d00b77051 100644
--- a/src/Components/Components/src/Routing/RouteConstraint.cs
+++ b/src/Components/Components/src/Routing/RouteConstraint.cs
@@ -2,15 +2,20 @@
 // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
 
 using System;
-using System.Collections.Generic;
+using System.Collections.Concurrent;
 using System.Globalization;
 
 namespace Microsoft.AspNetCore.Components.Routing
 {
     internal abstract class RouteConstraint
     {
-        private static readonly IDictionary<string, RouteConstraint> _cachedConstraints
-            = new Dictionary<string, RouteConstraint>();
+        // note: the things that prevent this cache from growing unbounded is that
+        // we're the only caller to this code path, and the fact that there are only
+        // 8 possible instances that we create.
+        //
+        // The values passed in here for parsing are always static text defined in route attributes.
+        private static readonly ConcurrentDictionary<string, RouteConstraint> _cachedConstraints
+            = new ConcurrentDictionary<string, RouteConstraint>();
 
         public abstract bool Match(string pathSegment, out object convertedValue);
 
@@ -30,7 +35,7 @@ public static RouteConstraint Parse(string template, string segment, string cons
                 var newInstance = CreateRouteConstraint(constraint);
                 if (newInstance != null)
                 {
-                    _cachedConstraints[constraint] = newInstance;
+                    _cachedConstraints.TryAdd(constraint, newInstance);
                     return newInstance;
                 }
                 else
diff --git a/src/Components/Components/test/Routing/RouteConstraintTest.cs b/src/Components/Components/test/Routing/RouteConstraintTest.cs
new file mode 100644
index 000000000000..34889f03dd46
--- /dev/null
+++ b/src/Components/Components/test/Routing/RouteConstraintTest.cs
@@ -0,0 +1,36 @@
+// Copyright (c) .NET Foundation. All rights reserved.
+// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
+
+using Xunit;
+
+namespace Microsoft.AspNetCore.Components.Routing
+{
+    public class RouteConstraintTest
+    {
+        [Fact]
+        public void Parse_CreatesDifferentConstraints_ForDifferentKinds()
+        {
+            // Arrange
+            var original = RouteConstraint.Parse("ignore", "ignore", "int");
+
+            // Act
+            var another = RouteConstraint.Parse("ignore", "ignore", "guid");
+
+            // Assert
+            Assert.NotSame(original, another);
+        }
+
+        [Fact]
+        public void Parse_CachesCreatedConstraint_ForSameKind()
+        {
+            // Arrange
+            var original = RouteConstraint.Parse("ignore", "ignore", "int");
+
+            // Act
+            var another = RouteConstraint.Parse("ignore", "ignore", "int");
+
+            // Assert
+            Assert.Same(original, another);
+        }
+    }
+}