diff --git a/src/Microsoft.AspNet.Mvc.Razor/Compilation/CompilerCache.cs b/src/Microsoft.AspNet.Mvc.Razor/Compilation/CompilerCache.cs index da6bb51cf0..62788e7eee 100644 --- a/src/Microsoft.AspNet.Mvc.Razor/Compilation/CompilerCache.cs +++ b/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 _normalizedPathLookup = new ConcurrentDictionary(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 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 CreateCacheEntry( string normalizedPath, Func compile) { - var fileInfo = _fileProvider.GetFileInfo(normalizedPath); - MemoryCacheEntryOptions cacheEntryOptions; - CompilerCacheResult cacheResult; - if (!fileInfo.Exists) - { - var expirationToken = _fileProvider.Watch(normalizedPath); - cacheResult = new CompilerCacheResult(new[] { expirationToken }); + TaskCompletionSource compilationTaskSource = null; + MemoryCacheEntryOptions cacheEntryOptions = null; + IFileInfo fileInfo = null; + Task cacheEntry; - cacheEntryOptions = new MemoryCacheEntryOptions(); - cacheEntryOptions.AddExpirationToken(expirationToken); - } - else + // Safe races cannot be allowed when compiling Razor pages. To ensure only one compilation request succeeds + // 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. + 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(); + cacheEntry = compilationTaskSource.Task; + } + + cacheEntry = _cache.Set>(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( - compilationResult, - cacheEntryOptions.ExpirationTokens); + + compilationTaskSource.SetResult( + new CompilerCacheResult(compilationResult, cacheEntryOptions.ExpirationTokens)); } - _cache.Set(normalizedPath, cacheResult, cacheEntryOptions); - return cacheResult; + return cacheEntry; } private MemoryCacheEntryOptions GetMemoryCacheEntryOptions(string relativePath) diff --git a/test/Microsoft.AspNet.Mvc.Razor.Test/Compilation/CompilerCacheTest.cs b/test/Microsoft.AspNet.Mvc.Razor.Test/Compilation/CompilerCacheTest.cs index 3203c8920c..f0839969a8 100644 --- a/test/Microsoft.AspNet.Mvc.Razor.Test/Compilation/CompilerCacheTest.cs +++ b/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 { }