CompilerCache should not compile multiple assemblies for the same page.

Fixes #3469

src/Microsoft.AspNet.Mvc.Razor/Compilation/CompilerCache.cs

@@ -6,9 +6,9 @@ using System.Collections.Concurrent;
 using System.Collections.Generic;
 using System.Diagnostics;
 using System.Text;
+using System.Threading.Tasks;
 using Microsoft.AspNet.FileProviders;
 using Microsoft.Extensions.Caching.Memory;
-using Microsoft.Extensions.Primitives;
 
 namespace Microsoft.AspNet.Mvc.Razor.Compilation
 {
@@ -19,6 +19,7 @@ namespace Microsoft.AspNet.Mvc.Razor.Compilation
     {
         private readonly IFileProvider _fileProvider;
         private readonly IMemoryCache _cache;
+        private readonly object _cacheLock = new object();
 
         private readonly ConcurrentDictionary<string, string> _normalizedPathLookup =
             new ConcurrentDictionary<string, string>(StringComparer.Ordinal);
@@ -58,7 +59,7 @@ namespace Microsoft.AspNet.Mvc.Razor.Compilation
             foreach (var item in precompiledViews)
             {
                 var cacheEntry = new CompilerCacheResult(new CompilationResult(item.Value));
-                _cache.Set(GetNormalizedPath(item.Key), cacheEntry);
+                _cache.Set(GetNormalizedPath(item.Key), Task.FromResult(cacheEntry));
             }
         }
 
@@ -77,49 +78,77 @@ namespace Microsoft.AspNet.Mvc.Razor.Compilation
                 throw new ArgumentNullException(nameof(compile));
             }
 
-            CompilerCacheResult cacheResult;
+            Task<CompilerCacheResult> cacheEntry;
             // Attempt to lookup the cache entry using the passed in path. This will succeed if the path is already
             // normalized and a cache entry exists.
-            if (!_cache.TryGetValue(relativePath, out cacheResult))
+            if (!_cache.TryGetValue(relativePath, out cacheEntry))
             {
                 var normalizedPath = GetNormalizedPath(relativePath);
-                if (!_cache.TryGetValue(normalizedPath, out cacheResult))
+                if (!_cache.TryGetValue(normalizedPath, out cacheEntry))
                 {
-                    cacheResult = CreateCacheEntry(normalizedPath, compile);
+                    cacheEntry = CreateCacheEntry(normalizedPath, compile);
                 }
             }
 
-            return cacheResult;
+            // The Task does not represent async work and is meant to provide per-entry locking.
+            // Hence it is ok to perform .GetResult() to read the result.
+            return cacheEntry.GetAwaiter().GetResult();
         }
 
-        private CompilerCacheResult CreateCacheEntry(
+        private Task<CompilerCacheResult> CreateCacheEntry(
             string normalizedPath,
             Func<RelativeFileInfo, CompilationResult> compile)
         {
-            var fileInfo = _fileProvider.GetFileInfo(normalizedPath);
+            TaskCompletionSource<CompilerCacheResult> compilationTaskSource = null;
-            MemoryCacheEntryOptions cacheEntryOptions;
+            MemoryCacheEntryOptions cacheEntryOptions = null;
-            CompilerCacheResult cacheResult;
+            IFileInfo fileInfo = null;
-            if (!fileInfo.Exists)
+            Task<CompilerCacheResult> cacheEntry;
-            {
-                var expirationToken = _fileProvider.Watch(normalizedPath);
-                cacheResult = new CompilerCacheResult(new[] { expirationToken });
 
-                cacheEntryOptions = new MemoryCacheEntryOptions();
+            // Safe races cannot be allowed when compiling Razor pages. To ensure only one compilation request succeeds
-                cacheEntryOptions.AddExpirationToken(expirationToken);
+            // per file, we'll lock the creation of a cache entry. Creating the cache entry should be very quick. The
-            }
+            // actual work for compiling files happens outside the critical section.
-            else
+            lock (_cacheLock)
             {
+                if (_cache.TryGetValue(normalizedPath, out cacheEntry))
+                {
+                    return cacheEntry;
+                }
+
+                fileInfo = _fileProvider.GetFileInfo(normalizedPath);
+                if (!fileInfo.Exists)
+                {
+                    var expirationToken = _fileProvider.Watch(normalizedPath);
+                    cacheEntry = Task.FromResult(new CompilerCacheResult(new[] { expirationToken }));
+
+                    cacheEntryOptions = new MemoryCacheEntryOptions();
+                    cacheEntryOptions.AddExpirationToken(expirationToken);
+                }
+                else
+                {
+                    cacheEntryOptions = GetMemoryCacheEntryOptions(normalizedPath);
+
+                    // A file exists and needs to be compiled.
+                    compilationTaskSource = new TaskCompletionSource<CompilerCacheResult>();
+                    cacheEntry = compilationTaskSource.Task;
+                }
+
+                cacheEntry = _cache.Set<Task<CompilerCacheResult>>(normalizedPath, cacheEntry, cacheEntryOptions);
+            }
+
+            if (compilationTaskSource != null)
+            {
+                // Indicates that the file was found and needs to be compiled.
+                Debug.Assert(fileInfo != null && fileInfo.Exists);
+                Debug.Assert(cacheEntryOptions != null);
                 var relativeFileInfo = new RelativeFileInfo(fileInfo, normalizedPath);
                 var compilationResult = compile(relativeFileInfo);
                 compilationResult.EnsureSuccessful();
-                cacheEntryOptions = GetMemoryCacheEntryOptions(normalizedPath);
+
-                cacheResult = new CompilerCacheResult(
+                compilationTaskSource.SetResult(
-                    compilationResult,
+                    new CompilerCacheResult(compilationResult, cacheEntryOptions.ExpirationTokens));
-                    cacheEntryOptions.ExpirationTokens);
             }
 
-            _cache.Set(normalizedPath, cacheResult, cacheEntryOptions);
+            return cacheEntry;
-            return cacheResult;
         }
 
         private MemoryCacheEntryOptions GetMemoryCacheEntryOptions(string relativePath)

test/Microsoft.AspNet.Mvc.Razor.Test/Compilation/CompilerCacheTest.cs

@@ -3,6 +3,8 @@
 
 using System;
 using System.Collections.Generic;
+using System.Threading;
+using System.Threading.Tasks;
 using Moq;
 using Xunit;
 
@@ -334,6 +336,113 @@ namespace Microsoft.AspNet.Mvc.Razor.Compilation
             Assert.Same(expected, result.CompilationResult.CompiledType);
         }
 
+        [Fact]
+        public async Task GetOrAdd_AllowsConcurrentCompilationOfMultipleRazorPages()
+        {
+            // Arrange
+            var waitDuration = TimeSpan.FromSeconds(20);
+            var fileProvider = new TestFileProvider();
+            fileProvider.AddFile("/Views/Home/Index.cshtml", "Index content");
+            fileProvider.AddFile("/Views/Home/About.cshtml", "About content");
+            var resetEvent1 = new AutoResetEvent(initialState: false);
+            var resetEvent2 = new ManualResetEvent(initialState: false);
+            var cache = new CompilerCache(fileProvider);
+            var compilingOne = false;
+            var compilingTwo = false;
+
+            // Act
+            var task1 = Task.Run(() =>
+            {
+                return cache.GetOrAdd("/Views/Home/Index.cshtml", file =>
+                {
+                    compilingOne = true;
+
+                    // Event 2
+                    resetEvent1.WaitOne(waitDuration);
+
+                    // Event 3
+                    resetEvent2.Set();
+
+                    // Event 6
+                    resetEvent1.WaitOne(waitDuration);
+
+                    Assert.True(compilingTwo);
+                    return new CompilationResult(typeof(TestView));
+                });
+            });
+
+            var task2 = Task.Run(() =>
+            {
+                // Event 4
+                return cache.GetOrAdd("/Views/Home/About.cshtml", file =>
+                {
+                    compilingTwo = true;
+
+                    // Event 4
+                    resetEvent2.WaitOne(waitDuration);
+
+                    // Event 5
+                    resetEvent1.Set();
+
+                    Assert.True(compilingOne);
+                    return new CompilationResult(typeof(DifferentView));
+                });
+            });
+
+            // Event 1
+            resetEvent1.Set();
+
+            await Task.WhenAll(task1, task2);
+
+            // Assert
+            var result1 = task1.Result;
+            var result2 = task2.Result;
+            Assert.True(compilingOne);
+            Assert.True(compilingTwo);
+        }
+
+        [Fact]
+        public async Task GetOrAdd_DoesNotCreateMultipleCompilationResults_ForConcurrentInvocations()
+        {
+            // Arrange
+            var waitDuration = TimeSpan.FromSeconds(20);
+            var fileProvider = new TestFileProvider();
+            fileProvider.AddFile(ViewPath, "some content");
+            var resetEvent1 = new ManualResetEvent(initialState: false);
+            var resetEvent2 = new ManualResetEvent(initialState: false);
+            var cache = new CompilerCache(fileProvider);
+
+            // Act
+            var task1 = Task.Run(() =>
+            {
+                return cache.GetOrAdd(ViewPath, file =>
+                {
+                    // Event 2
+                    resetEvent1.WaitOne(waitDuration);
+
+                    // Event 3
+                    resetEvent2.Set();
+                    return new CompilationResult(typeof(TestView));
+                });
+            });
+
+            var task2 = Task.Run(() =>
+            {
+                // Event 4
+                resetEvent2.WaitOne(waitDuration);
+                return cache.GetOrAdd(ViewPath, ThrowsIfCalled);
+            });
+
+            // Event 1
+            resetEvent1.Set();
+            await Task.WhenAll(task1, task2);
+
+            // Assert
+            var result1 = task1.Result;
+            var result2 = task2.Result;
+            Assert.Same(result1.CompilationResult.CompiledType, result2.CompilationResult.CompiledType);
+        }
+
         private class TestView
         {
         }