From 50cef4822a5fda5490896818377a15611246f703 Mon Sep 17 00:00:00 2001 From: Pranav K Date: Mon, 24 Sep 2018 10:46:54 -0700 Subject: [PATCH] Invoke FlushAsync before disposing the HttpResponseWriter in JsonResultExecutor Fixes #8486 --- .../Internal/JsonResultExecutor.cs | 8 ++- .../Internal/JsonResultExecutorTest.cs | 62 +++++++++++++++++++ 2 files changed, 67 insertions(+), 3 deletions(-) diff --git a/src/Microsoft.AspNetCore.Mvc.Formatters.Json/Internal/JsonResultExecutor.cs b/src/Microsoft.AspNetCore.Mvc.Formatters.Json/Internal/JsonResultExecutor.cs index f7e2fc09f2..332d1bdc59 100644 --- a/src/Microsoft.AspNetCore.Mvc.Formatters.Json/Internal/JsonResultExecutor.cs +++ b/src/Microsoft.AspNetCore.Mvc.Formatters.Json/Internal/JsonResultExecutor.cs @@ -86,7 +86,7 @@ namespace Microsoft.AspNetCore.Mvc.Formatters.Json.Internal /// The . /// The . /// A which will complete when writing has completed. - public virtual Task ExecuteAsync(ActionContext context, JsonResult result) + public virtual async Task ExecuteAsync(ActionContext context, JsonResult result) { if (context == null) { @@ -128,9 +128,11 @@ namespace Microsoft.AspNetCore.Mvc.Formatters.Json.Internal var jsonSerializer = JsonSerializer.Create(serializerSettings); jsonSerializer.Serialize(jsonWriter, result.Value); } - } - return Task.CompletedTask; + // Perf: call FlushAsync to call WriteAsync on the stream with any content left in the TextWriter's + // buffers. This is better than just letting dispose handle it (which would result in a synchronous write). + await writer.FlushAsync(); + } } } } diff --git a/test/Microsoft.AspNetCore.Mvc.Formatters.Json.Test/Internal/JsonResultExecutorTest.cs b/test/Microsoft.AspNetCore.Mvc.Formatters.Json.Test/Internal/JsonResultExecutorTest.cs index 2dbbeb1dad..8803474ccd 100644 --- a/test/Microsoft.AspNetCore.Mvc.Formatters.Json.Test/Internal/JsonResultExecutorTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.Formatters.Json.Test/Internal/JsonResultExecutorTest.cs @@ -5,6 +5,7 @@ using System; using System.Buffers; using System.IO; using System.Text; +using System.Threading; using System.Threading.Tasks; using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Mvc.Abstractions; @@ -14,6 +15,7 @@ using Microsoft.Extensions.Logging; using Microsoft.Extensions.Logging.Abstractions; using Microsoft.Extensions.Options; using Microsoft.Net.Http.Headers; +using Moq; using Newtonsoft.Json; using Xunit; @@ -217,6 +219,66 @@ namespace Microsoft.AspNetCore.Mvc.Formatters.Json.Internal Assert.Equal(expected, logger.MostRecentMessage); } + [Fact] + public async Task ExecuteAsync_WritesToTheResponseStream_WhenContentIsLargerThanBuffer() + { + // Arrange + var writeLength = 2 * TestHttpResponseStreamWriterFactory.DefaultBufferSize + 4; + var text = new string('a', writeLength); + var expectedWriteCallCount = Math.Ceiling((double)writeLength / TestHttpResponseStreamWriterFactory.DefaultBufferSize); + + var stream = new Mock(); + stream.SetupGet(s => s.CanWrite).Returns(true); + var httpContext = new DefaultHttpContext(); + httpContext.Response.Body = stream.Object; + var actionContext = new ActionContext(httpContext, new RouteData(), new ActionDescriptor()); + + var result = new JsonResult(text); + var executor = CreateExecutor(); + + // Act + await executor.ExecuteAsync(actionContext, result); + + // Assert + // HttpResponseStreamWriter buffers content up to the buffer size (16k). When writes exceed the buffer size, it'll perform a synchronous + // write to the response stream. + stream.Verify(s => s.Write(It.IsAny(), It.IsAny(), TestHttpResponseStreamWriterFactory.DefaultBufferSize), Times.Exactly(2)); + + // Remainder buffered content is written asynchronously as part of the FlushAsync. + stream.Verify(s => s.WriteAsync(It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny()), Times.Once()); + + // Dispose does not call Flush + stream.Verify(s => s.Flush(), Times.Never()); + } + + [Theory] + [InlineData(5)] + [InlineData(TestHttpResponseStreamWriterFactory.DefaultBufferSize - 30)] + public async Task ExecuteAsync_DoesNotWriteSynchronouslyToTheResponseBody_WhenContentIsSmallerThanBufferSize(int writeLength) + { + // Arrange + var text = new string('a', writeLength); + + var stream = new Mock(); + stream.SetupGet(s => s.CanWrite).Returns(true); + var httpContext = new DefaultHttpContext(); + httpContext.Response.Body = stream.Object; + var actionContext = new ActionContext(httpContext, new RouteData(), new ActionDescriptor()); + + var result = new JsonResult(text); + var executor = CreateExecutor(); + + // Act + await executor.ExecuteAsync(actionContext, result); + + // Assert + // HttpResponseStreamWriter buffers content up to the buffer size (16k) and will asynchronously write content to the response as part + // of the FlushAsync call if the content written to it is smaller than the buffer size. + // This test verifies that no synchronous writes are performed in this scenario. + stream.Verify(s => s.Flush(), Times.Never()); + stream.Verify(s => s.Write(It.IsAny(), It.IsAny(), It.IsAny()), Times.Never()); + } + private static JsonResultExecutor CreateExecutor(ILogger logger = null) { return new JsonResultExecutor(