Skip to content

Commit

Permalink
Make constraint cache thread safe (#17146)
Browse files Browse the repository at this point in the history
* 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.

* PR feedback
  • Loading branch information
rynowak authored and wtgodbe committed Nov 16, 2019
1 parent 7ceadd0 commit 8f8b5f5
Show file tree
Hide file tree
Showing 2 changed files with 48 additions and 5 deletions.
17 changes: 12 additions & 5 deletions src/Components/Components/src/Routing/RouteConstraint.cs
Expand Up @@ -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);

Expand All @@ -30,8 +35,10 @@ public static RouteConstraint Parse(string template, string segment, string cons
var newInstance = CreateRouteConstraint(constraint);
if (newInstance != null)
{
_cachedConstraints[constraint] = newInstance;
return newInstance;
// We've done to the work to create the constraint now, but it's possible
// we're competing with another thread. GetOrAdd can ensure only a single
// instance is returned so that any extra ones can be GC'ed.
return _cachedConstraints.GetOrAdd(constraint, newInstance);
}
else
{
Expand Down
36 changes: 36 additions & 0 deletions 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);
}
}
}

0 comments on commit 8f8b5f5

Please sign in to comment.