Avoid saving TempData in case of unhandled exceptions. [Fixes #6598] BUG? Accessing TempData prevent response to have content on error.

src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/SaveTempDataFilter.cs

@@ -1,6 +1,7 @@
 // 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 System;
 using System.Collections.Generic;
 using System.Threading.Tasks;
 using Microsoft.AspNetCore.Http;
@@ -14,7 +15,7 @@ namespace Microsoft.AspNetCore.Mvc.ViewFeatures.Internal
     public class SaveTempDataFilter : IResourceFilter, IResultFilter
     {
         // Internal for unit testing
-        internal static readonly object TempDataSavedKey = new object();
+        internal static readonly object SaveTempDataFilterContextKey = new object();
 
         private readonly ITempDataDictionaryFactory _factory;
 
@@ -30,18 +31,34 @@ namespace Microsoft.AspNetCore.Mvc.ViewFeatures.Internal
         /// <inheritdoc />
         public void OnResourceExecuting(ResourceExecutingContext context)
         {
+            if (!context.HttpContext.Items.ContainsKey(SaveTempDataFilterContextKey))
+            {
+                var tempDataContext = new SaveTempDataContext()
+                {
+                    Filters = context.Filters,
+                    TempDataDictionaryFactory = _factory
+                };
+                context.HttpContext.Items.Add(SaveTempDataFilterContextKey, tempDataContext);
+            }
+
             if (!context.HttpContext.Response.HasStarted)
             {
                 context.HttpContext.Response.OnStarting((state) =>
                 {
-                    var saveTempDataContext = (SaveTempDataContext)state;
+                    var httpContext = (HttpContext)state;
+
+                    var saveTempDataContext = GetTempDataContext(context.HttpContext);
+                    if (saveTempDataContext.RequestHasUnhandledException)
+                    {
+                        return Task.CompletedTask;
+                    }
 
                     // If temp data was already saved, skip trying to save again as the calls here would potentially fail
                     // because the session feature might not be available at this point.
                     // Example: An action returns NoContentResult and since NoContentResult does not write anything to
                     // the body of the response, this delegate would get executed way late in the pipeline at which point
                     // the session feature would have been removed.
-                    if (saveTempDataContext.HttpContext.Items.TryGetValue(TempDataSavedKey, out var obj))
+                    if (saveTempDataContext.TempDataSaved)
                     {
                         return Task.CompletedTask;
                     }
@@ -50,22 +67,29 @@ namespace Microsoft.AspNetCore.Mvc.ViewFeatures.Internal
                         result: null,
                         factory: saveTempDataContext.TempDataDictionaryFactory,
                         filters: saveTempDataContext.Filters,
-                        httpContext: saveTempDataContext.HttpContext);
+                        httpContext: httpContext);
 
                     return Task.CompletedTask;
                 },
-                state: new SaveTempDataContext()
-                {
-                    Filters = context.Filters,
-                    HttpContext = context.HttpContext,
-                    TempDataDictionaryFactory = _factory
-                });
+                state: context.HttpContext);
             }
         }
 
         /// <inheritdoc />
         public void OnResourceExecuted(ResourceExecutedContext context)
         {
+            // If there is an unhandled exception, we would like to avoid setting tempdata as 
+            // the end user is going to see an error page anyway and also it helps us in avoiding
+            // accessing resources like Session too late in the request lifecyle where SessionFeature might
+            // not be available.
+            if (!context.HttpContext.Response.HasStarted && context.Exception != null)
+            {
+                var saveTempDataContext = GetTempDataContext(context.HttpContext);
+                if (saveTempDataContext != null)
+                {
+                    saveTempDataContext.RequestHasUnhandledException = true;
+                }
+            }
         }
 
         /// <inheritdoc />
@@ -82,14 +106,25 @@ namespace Microsoft.AspNetCore.Mvc.ViewFeatures.Internal
             if (!context.HttpContext.Response.HasStarted)
             {
                 SaveTempData(context.Result, _factory, context.Filters, context.HttpContext);
-                // If SaveTempDataFilter got added twice this might already be in there.
-                if (!context.HttpContext.Items.ContainsKey(TempDataSavedKey))
+
+                var saveTempDataContext = GetTempDataContext(context.HttpContext);
+                if (saveTempDataContext != null)
                 {
-                    context.HttpContext.Items.Add(TempDataSavedKey, true);
+                    saveTempDataContext.TempDataSaved = true;
                 }
             }
         }
 
+        private SaveTempDataContext GetTempDataContext(HttpContext httpContext)
+        {
+            SaveTempDataContext saveTempDataContext = null;
+            if (httpContext.Items.TryGetValue(SaveTempDataFilterContextKey, out var value))
+            {
+                saveTempDataContext = (SaveTempDataContext)value;
+            }
+            return saveTempDataContext;
+        }
+
         private static void SaveTempData(
             IActionResult result,
             ITempDataDictionaryFactory factory,
@@ -114,10 +149,11 @@ namespace Microsoft.AspNetCore.Mvc.ViewFeatures.Internal
             tempData.Save();
         }
 
-        private class SaveTempDataContext
+        internal class SaveTempDataContext
         {
+            public bool RequestHasUnhandledException { get; set; }
+            public bool TempDataSaved { get; set; }
             public IList<IFilterMetadata> Filters { get; set; }
-            public HttpContext HttpContext { get; set; }
             public ITempDataDictionaryFactory TempDataDictionaryFactory { get; set; }
         }
     }

test/Microsoft.AspNetCore.Mvc.FunctionalTests/TempDataTestBase.cs

@@ -207,6 +207,42 @@ namespace Microsoft.AspNetCore.Mvc.FunctionalTests
             Assert.Equal(HttpStatusCode.NoContent, response.StatusCode);
         }
 
+        [Fact]
+        public async Task SaveTempDataFilter_DoesNotSaveTempData_OnUnhandledException()
+        {
+            // Arrange & Act
+            var response = await Client.GetAsync("/TempData/UnhandledExceptionAndSettingTempData");
+
+            // Assert
+            Assert.Equal(HttpStatusCode.InternalServerError, response.StatusCode);
+            var responseBody = await response.Content.ReadAsStringAsync();
+            Assert.Contains("Exception from action UnhandledExceptionAndSettingTempData", responseBody);
+
+            // Arrange & Act
+            response = await Client.GetAsync("/TempData/UnhandledExceptionAndGetTempData");
+
+            // Assert
+            Assert.Equal(HttpStatusCode.NoContent, response.StatusCode);
+        }
+
+        [Fact]
+        public async Task SaveTempDataFilter_DoesNotSaveTempData_OnHandledExceptions()
+        {
+            // Arrange & Act
+            var response = await Client.GetAsync("/TempData/UnhandledExceptionAndSettingTempData?handleException=true");
+
+            // Assert
+            Assert.Equal(HttpStatusCode.OK, response.StatusCode);
+            var responseBody = await response.Content.ReadAsStringAsync();
+            Assert.Contains("Exception was handled in TestExceptionFilter", responseBody);
+
+            // Arrange & Act
+            response = await Client.GetAsync("/TempData/UnhandledExceptionAndGetTempData");
+
+            // Assert
+            Assert.Equal(HttpStatusCode.NoContent, response.StatusCode);
+        }
+
         public HttpRequestMessage GetRequest(string path, HttpResponseMessage response)
         {
             var request = new HttpRequestMessage(HttpMethod.Get, path);

test/Microsoft.AspNetCore.Mvc.ViewFeatures.Test/Internal/SaveTempDataFilterTest.cs

@@ -102,7 +102,29 @@ namespace Microsoft.AspNetCore.Mvc.ViewFeatures.Internal
             // Arrange
             var responseFeature = new TestResponseFeature();
             var httpContext = GetHttpContext(responseFeature);
-            httpContext.Items[SaveTempDataFilter.TempDataSavedKey] = true; // indicate that tempdata was already saved
+            httpContext.Items[SaveTempDataFilter.SaveTempDataFilterContextKey] = new SaveTempDataFilter.SaveTempDataContext() { TempDataSaved = true };
+            var tempDataFactory = new Mock<ITempDataDictionaryFactory>(MockBehavior.Strict);
+            tempDataFactory
+                .Setup(f => f.GetTempData(It.IsAny<HttpContext>()))
+                .Verifiable();
+            var filter = new SaveTempDataFilter(tempDataFactory.Object);
+            var context = GetResourceExecutingContext(httpContext);
+            filter.OnResourceExecuting(context); // registers callback
+
+            // Act
+            await responseFeature.FireOnSendingHeadersAsync();
+
+            // Assert
+            tempDataFactory.Verify(tdf => tdf.GetTempData(It.IsAny<HttpContext>()), Times.Never());
+        }
+
+        [Fact]
+        public async Task OnResourceExecuting_DoesNotSaveTempData_WhenUnhandledExceptionOccurs()
+        {
+            // Arrange
+            var responseFeature = new TestResponseFeature();
+            var httpContext = GetHttpContext(responseFeature);
+            httpContext.Items[SaveTempDataFilter.SaveTempDataFilterContextKey] = new SaveTempDataFilter.SaveTempDataContext() { RequestHasUnhandledException = true };
             var tempDataFactory = new Mock<ITempDataDictionaryFactory>(MockBehavior.Strict);
             tempDataFactory
                 .Setup(f => f.GetTempData(It.IsAny<HttpContext>()))

test/WebSites/BasicWebSite/BasicWebSite.csproj

@@ -13,5 +13,6 @@
     <PackageReference Include="Microsoft.AspNetCore.Server.Kestrel" />
     <PackageReference Include="Microsoft.AspNetCore.Session" />
     <PackageReference Include="Microsoft.AspNetCore.StaticFiles" />
+    <PackageReference Include="Microsoft.AspNetCore.Diagnostics" />
   </ItemGroup>
 </Project>

test/WebSites/BasicWebSite/Controllers/TempDataController.cs

@@ -4,6 +4,7 @@
 using System;
 using System.Collections.Generic;
 using System.Threading.Tasks;
+using BasicWebSite.Filters;
 using Microsoft.AspNetCore.Http;
 using Microsoft.AspNetCore.Mvc;
 
@@ -102,5 +103,19 @@ namespace BasicWebSite.Controllers
         {
             return TempData["LargeValue"]?.ToString();
         }
+
+        [HttpGet]
+        [TestExceptionFilter]
+        public IActionResult UnhandledExceptionAndSettingTempData()
+        {
+            TempData[nameof(UnhandledExceptionAndSettingTempData)] = "James";
+            throw new InvalidOperationException($"Exception from action {nameof(UnhandledExceptionAndSettingTempData)}");
+        }
+
+        [HttpGet]
+        public string UnhandledExceptionAndGetTempData()
+        {
+            return TempData[nameof(UnhandledExceptionAndSettingTempData)]?.ToString();
+        }
     }
 }

test/WebSites/BasicWebSite/Filters/TestExceptionFilter.cs

@@ -0,0 +1,23 @@
+// 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 Microsoft.AspNetCore.Mvc;
+using Microsoft.AspNetCore.Mvc.Filters;
+
+namespace BasicWebSite.Filters
+{
+    public class TestExceptionFilter : ExceptionFilterAttribute
+    {
+        public override void OnException(ExceptionContext context)
+        {
+            if (context.HttpContext.Request.Query.TryGetValue("handleException", out var handleException))
+            {
+                if (handleException.Equals("true"))
+                {
+                    context.Result = new ContentResult() { Content = "Exception was handled in TestExceptionFilter", StatusCode = 200 };
+                    context.ExceptionHandled = true;
+                }
+            }
+        }
+    }
+}

test/WebSites/BasicWebSite/Startup.cs

@@ -28,6 +28,8 @@ namespace BasicWebSite
 
         public void Configure(IApplicationBuilder app)
         {
+            app.UseDeveloperExceptionPage();
+
             app.UseStaticFiles();
 
             // Initializes the RequestId service for each request

test/WebSites/BasicWebSite/StartupWithSessionTempDataProvider.cs

@@ -19,6 +19,7 @@ namespace BasicWebSite
 
         public void Configure(IApplicationBuilder app)
         {
+            app.UseDeveloperExceptionPage();
             app.UseSession();
             app.UseMvcWithDefaultRoute();
         }