From 9dc9381ae4cddf2661818014b6e95fae99019f4b Mon Sep 17 00:00:00 2001 From: Pranav K Date: Mon, 20 Nov 2017 09:08:55 -0800 Subject: [PATCH] Re-order execution in CacheTagHelper to avoid setting the result too early Re-enable skipped tests Fixes #7042 --- .../CacheTagHelper.cs | 25 +++++++++++-------- .../HtmlGenerationTest.cs | 4 +-- .../CacheTagHelperTest.cs | 2 +- 3 files changed, 17 insertions(+), 14 deletions(-) diff --git a/src/Microsoft.AspNetCore.Mvc.TagHelpers/CacheTagHelper.cs b/src/Microsoft.AspNetCore.Mvc.TagHelpers/CacheTagHelper.cs index e42bb67883..4a7bb989b7 100644 --- a/src/Microsoft.AspNetCore.Mvc.TagHelpers/CacheTagHelper.cs +++ b/src/Microsoft.AspNetCore.Mvc.TagHelpers/CacheTagHelper.cs @@ -108,8 +108,9 @@ namespace Microsoft.AspNetCore.Mvc.TagHelpers // The returned value is ignored, we only do this so that // the compiler doesn't complain about the returned task // not being awaited - var localTcs = MemoryCache.Set(cacheKey, tcs.Task, options); + _ = MemoryCache.Set(cacheKey, tcs.Task, options); + IHtmlContent content; try { // The entry is set instead of assigning a value to the @@ -118,24 +119,26 @@ namespace Microsoft.AspNetCore.Mvc.TagHelpers // Use the CreateEntry to ensure a cache scope is created that will copy expiration tokens from // cache entries created from the GetChildContentAsync call to the current entry. - IHtmlContent content; var entry = MemoryCache.CreateEntry(cacheKey); - // The result is processed inside an entry - // such that the tokens are inherited. + // The result is processed inside an entry + // such that the tokens are inherited. - var result = ProcessContentAsync(output); - content = await result; - options.SetSize(GetSize(content)); - entry.SetOptions(options); + var result = ProcessContentAsync(output); + content = await result; + options.SetSize(GetSize(content)); + entry.SetOptions(options); - entry.Value = result; + entry.Value = result; - tcs.SetResult(content); // An entry gets committed to the cache when disposed gets called. We only want to do this when // the content has been correctly generated (didn't throw an exception). For that reason the entry // can't be put inside a using block. entry.Dispose(); + + // Set the result on the TCS once we've commited the entry to the cache since commiting to the cache + // may throw. + tcs.SetResult(content); return content; } catch (Exception ex) @@ -144,7 +147,7 @@ namespace Microsoft.AspNetCore.Mvc.TagHelpers tokenSource.Cancel(); // Fail the TCS so other awaiters see the exception. - tcs.SetException(ex); + tcs.TrySetException(ex); throw; } finally diff --git a/test/Microsoft.AspNetCore.Mvc.FunctionalTests/HtmlGenerationTest.cs b/test/Microsoft.AspNetCore.Mvc.FunctionalTests/HtmlGenerationTest.cs index 2d6e5d0faf..1579544e7c 100644 --- a/test/Microsoft.AspNetCore.Mvc.FunctionalTests/HtmlGenerationTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.FunctionalTests/HtmlGenerationTest.cs @@ -255,7 +255,7 @@ namespace Microsoft.AspNetCore.Mvc.FunctionalTests #endif } - [Fact(Skip = "https://github.com/aspnet/Mvc/issues/7042")] + [Fact] public async Task CacheTagHelper_CanCachePortionsOfViewsPartialViewsAndViewComponents() { // Arrange @@ -440,7 +440,7 @@ namespace Microsoft.AspNetCore.Mvc.FunctionalTests Assert.Equal(expected2, response4.Trim()); } - [Fact(Skip = "https://github.com/aspnet/Mvc/issues/7042")] + [Fact] public async Task CacheTagHelper_BubblesExpirationOfNestedTagHelpers() { // Arrange & Act - 1 diff --git a/test/Microsoft.AspNetCore.Mvc.TagHelpers.Test/CacheTagHelperTest.cs b/test/Microsoft.AspNetCore.Mvc.TagHelpers.Test/CacheTagHelperTest.cs index abcb437274..589ce007f8 100644 --- a/test/Microsoft.AspNetCore.Mvc.TagHelpers.Test/CacheTagHelperTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.TagHelpers.Test/CacheTagHelperTest.cs @@ -801,7 +801,7 @@ namespace Microsoft.AspNetCore.Mvc.TagHelpers Assert.Equal(childContent, tagHelperOutput4.Content.GetContent()); } - [Fact(Skip = "https://github.com/aspnet/Mvc/issues/7042")] + [Fact] public async Task ProcessAsync_WorksForNestedCacheTagHelpers() { // Arrange