From c304984a8d36c063fa549bde3d81b056d9d89136 Mon Sep 17 00:00:00 2001 From: Ryan Nowak Date: Thu, 7 Jan 2016 13:14:41 -0800 Subject: [PATCH] Fix #3501 - Issues with Flush Calling Flush[Async]() on the writer will NOT flush the stream. Calling Flush[Async]() in Razor will flush both the writer and the stream. Our normal flow will be to flush the writer, but not the stream. This avoids chunking, but allows us to do a WriteAsync on the stream as part of the call to FlushAsync. This is done to avoid a synchronous write due to Dispose calling Flush on the writer, which needs to call Write on the stream. See issue for extensive background. --- .../HttpResponseStreamWriter.cs | 30 +++++------- .../JsonOutputFormatter.cs | 9 ++-- ...mlDataContractSerializerOutputFormatter.cs | 9 ++-- .../XmlSerializerOutputFormatter.cs | 9 ++-- src/Microsoft.AspNet.Mvc.Razor/RazorPage.cs | 5 +- .../HttpResponseStreamWriterTest.cs | 47 +++++++++++++------ .../ViewFeatures/ViewExecutorTest.cs | 7 ++- 7 files changed, 71 insertions(+), 45 deletions(-) diff --git a/src/Microsoft.AspNet.Mvc.Core/HttpResponseStreamWriter.cs b/src/Microsoft.AspNet.Mvc.Core/HttpResponseStreamWriter.cs index 06a9dc6c9b..6b699b0c72 100644 --- a/src/Microsoft.AspNet.Mvc.Core/HttpResponseStreamWriter.cs +++ b/src/Microsoft.AspNet.Mvc.Core/HttpResponseStreamWriter.cs @@ -272,7 +272,7 @@ namespace Microsoft.AspNet.Mvc throw new ObjectDisposedException("stream"); } - FlushInternal(true, true); + FlushInternal(); } public override Task FlushAsync() @@ -282,18 +282,16 @@ namespace Microsoft.AspNet.Mvc throw new ObjectDisposedException("stream"); } - return FlushInternalAsync(flushStream: true, flushEncoder: true); + return FlushInternalAsync(); } - // Do not flush the stream on Dispose, as this will cause response to be - // sent in chunked encoding in case of Helios. protected override void Dispose(bool disposing) { if (disposing && _stream != null) { try { - FlushInternal(flushStream: false, flushEncoder: true); + FlushInternal(); } finally { @@ -314,7 +312,9 @@ namespace Microsoft.AspNet.Mvc } } - private void FlushInternal(bool flushStream = false, bool flushEncoder = false) + // Note: our FlushInternal method does NOT flush the underlying stream. This would result in + // chunking. + private void FlushInternal() { if (_charBufferCount == 0) { @@ -327,7 +327,7 @@ namespace Microsoft.AspNet.Mvc _charBufferCount, _byteBuffer, 0, - flushEncoder); + flush: true); if (count > 0) { @@ -335,14 +335,11 @@ namespace Microsoft.AspNet.Mvc } _charBufferCount = 0; - - if (flushStream) - { - _stream.Flush(); - } } - private async Task FlushInternalAsync(bool flushStream = false, bool flushEncoder = false) + // Note: our FlushInternalAsync method does NOT flush the underlying stream. This would result in + // chunking. + private async Task FlushInternalAsync() { if (_charBufferCount == 0) { @@ -355,7 +352,7 @@ namespace Microsoft.AspNet.Mvc _charBufferCount, _byteBuffer, 0, - flushEncoder); + flush: true); if (count > 0) { @@ -363,11 +360,6 @@ namespace Microsoft.AspNet.Mvc } _charBufferCount = 0; - - if (flushStream) - { - await _stream.FlushAsync(); - } } private void CopyToCharBuffer(string value, ref int index, ref int count) diff --git a/src/Microsoft.AspNet.Mvc.Formatters.Json/JsonOutputFormatter.cs b/src/Microsoft.AspNet.Mvc.Formatters.Json/JsonOutputFormatter.cs index beb9a5426e..efc69a30fa 100644 --- a/src/Microsoft.AspNet.Mvc.Formatters.Json/JsonOutputFormatter.cs +++ b/src/Microsoft.AspNet.Mvc.Formatters.Json/JsonOutputFormatter.cs @@ -132,7 +132,7 @@ namespace Microsoft.AspNet.Mvc.Formatters return _serializer; } - public override Task WriteResponseBodyAsync(OutputFormatterWriteContext context) + public override async Task WriteResponseBodyAsync(OutputFormatterWriteContext context) { if (context == null) { @@ -145,9 +145,12 @@ namespace Microsoft.AspNet.Mvc.Formatters using (var writer = context.WriterFactory(response.Body, selectedEncoding)) { WriteObject(writer, context.Object); - } - return Task.FromResult(true); + // 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/src/Microsoft.AspNet.Mvc.Formatters.Xml/XmlDataContractSerializerOutputFormatter.cs b/src/Microsoft.AspNet.Mvc.Formatters.Xml/XmlDataContractSerializerOutputFormatter.cs index e333ee12a9..758652a3fe 100644 --- a/src/Microsoft.AspNet.Mvc.Formatters.Xml/XmlDataContractSerializerOutputFormatter.cs +++ b/src/Microsoft.AspNet.Mvc.Formatters.Xml/XmlDataContractSerializerOutputFormatter.cs @@ -179,7 +179,7 @@ namespace Microsoft.AspNet.Mvc.Formatters } /// - public override Task WriteResponseBodyAsync(OutputFormatterWriteContext context) + public override async Task WriteResponseBodyAsync(OutputFormatterWriteContext context) { if (context == null) { @@ -209,9 +209,12 @@ namespace Microsoft.AspNet.Mvc.Formatters { dataContractSerializer.WriteObject(xmlWriter, value); } - } - return TaskCache.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 textWriter.FlushAsync(); + } } /// diff --git a/src/Microsoft.AspNet.Mvc.Formatters.Xml/XmlSerializerOutputFormatter.cs b/src/Microsoft.AspNet.Mvc.Formatters.Xml/XmlSerializerOutputFormatter.cs index 58c281b729..bf088222f6 100644 --- a/src/Microsoft.AspNet.Mvc.Formatters.Xml/XmlSerializerOutputFormatter.cs +++ b/src/Microsoft.AspNet.Mvc.Formatters.Xml/XmlSerializerOutputFormatter.cs @@ -154,7 +154,7 @@ namespace Microsoft.AspNet.Mvc.Formatters } /// - public override Task WriteResponseBodyAsync(OutputFormatterWriteContext context) + public override async Task WriteResponseBodyAsync(OutputFormatterWriteContext context) { if (context == null) { @@ -186,9 +186,12 @@ namespace Microsoft.AspNet.Mvc.Formatters { xmlSerializer.Serialize(xmlWriter, value); } - } - return TaskCache.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 textWriter.FlushAsync(); + } } /// diff --git a/src/Microsoft.AspNet.Mvc.Razor/RazorPage.cs b/src/Microsoft.AspNet.Mvc.Razor/RazorPage.cs index fcde47da43..5be9ecb71e 100644 --- a/src/Microsoft.AspNet.Mvc.Razor/RazorPage.cs +++ b/src/Microsoft.AspNet.Mvc.Razor/RazorPage.cs @@ -812,8 +812,8 @@ namespace Microsoft.AspNet.Mvc.Razor } /// - /// Invokes on writing out any buffered - /// content to the . + /// Invokes on and + /// on the response stream, writing out any buffered content to the . /// /// A that represents the asynchronous flush operation and on /// completion returns . @@ -842,6 +842,7 @@ namespace Microsoft.AspNet.Mvc.Razor } await Output.FlushAsync(); + await Context.Response.Body.FlushAsync(); return HtmlString.Empty; } diff --git a/test/Microsoft.AspNet.Mvc.Core.Test/HttpResponseStreamWriterTest.cs b/test/Microsoft.AspNet.Mvc.Core.Test/HttpResponseStreamWriterTest.cs index 28913b2005..9217739cc9 100644 --- a/test/Microsoft.AspNet.Mvc.Core.Test/HttpResponseStreamWriterTest.cs +++ b/test/Microsoft.AspNet.Mvc.Core.Test/HttpResponseStreamWriterTest.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.Buffers; using System.IO; using System.Text; @@ -164,18 +165,21 @@ namespace Microsoft.AspNet.Mvc [InlineData(1024)] [InlineData(1050)] [InlineData(2048)] - public void FlushesBuffer_OnFlush(int byteLength) + public void FlushesBuffer_ButNotStream_OnFlush(int byteLength) { // Arrange var stream = new TestMemoryStream(); var writer = new HttpResponseStreamWriter(stream, Encoding.UTF8); writer.Write(new string('a', byteLength)); + var expectedWriteCount = Math.Ceiling((double)byteLength / HttpResponseStreamWriter.DefaultBufferSize); + // Act writer.Flush(); // Assert - Assert.Equal(1, stream.FlushCallCount); + Assert.Equal(0, stream.FlushCallCount); + Assert.Equal(expectedWriteCount, stream.WriteCallCount); Assert.Equal(byteLength, stream.Length); } @@ -199,18 +203,21 @@ namespace Microsoft.AspNet.Mvc [InlineData(1024)] [InlineData(1050)] [InlineData(2048)] - public async Task FlushesBuffer_OnFlushAsync(int byteLength) + public async Task FlushesBuffer_ButNotStream_OnFlushAsync(int byteLength) { // Arrange var stream = new TestMemoryStream(); var writer = new HttpResponseStreamWriter(stream, Encoding.UTF8); await writer.WriteAsync(new string('a', byteLength)); + var expectedWriteCount = Math.Ceiling((double)byteLength / HttpResponseStreamWriter.DefaultBufferSize); + // Act await writer.FlushAsync(); // Assert - Assert.Equal(1, stream.FlushAsyncCallCount); + Assert.Equal(0, stream.FlushAsyncCallCount); + Assert.Equal(expectedWriteCount, stream.WriteAsyncCallCount); Assert.Equal(byteLength, stream.Length); } @@ -401,30 +408,42 @@ namespace Microsoft.AspNet.Mvc private class TestMemoryStream : MemoryStream { - private int _flushCallCount; - private int _flushAsyncCallCount; - private int _disposeCallCount; + public int FlushCallCount { get; private set; } - public int FlushCallCount { get { return _flushCallCount; } } - - public int FlushAsyncCallCount { get { return _flushAsyncCallCount; } } + public int FlushAsyncCallCount { get; private set; } public int CloseCallCount { get; private set; } - public int DisposeCallCount { get { return _disposeCallCount; } } + public int DisposeCallCount { get; private set; } + + public int WriteCallCount { get; private set; } + + public int WriteAsyncCallCount { get; private set; } public override void Flush() { - _flushCallCount++; + FlushCallCount++; base.Flush(); } public override Task FlushAsync(CancellationToken cancellationToken) { - _flushAsyncCallCount++; + FlushAsyncCallCount++; return base.FlushAsync(cancellationToken); } + public override void Write(byte[] buffer, int offset, int count) + { + WriteCallCount++; + base.Write(buffer, offset, count); + } + + public override Task WriteAsync(byte[] buffer, int offset, int count, CancellationToken cancellationToken) + { + WriteAsyncCallCount++; + return base.WriteAsync(buffer, offset, count, cancellationToken); + } + #if DNX451 public override void Close() { @@ -435,7 +454,7 @@ namespace Microsoft.AspNet.Mvc protected override void Dispose(bool disposing) { - _disposeCallCount++; + DisposeCallCount++; base.Dispose(disposing); } } diff --git a/test/Microsoft.AspNet.Mvc.ViewFeatures.Test/ViewFeatures/ViewExecutorTest.cs b/test/Microsoft.AspNet.Mvc.ViewFeatures.Test/ViewFeatures/ViewExecutorTest.cs index 5ad5239d53..b7a09d8e4a 100644 --- a/test/Microsoft.AspNet.Mvc.ViewFeatures.Test/ViewFeatures/ViewExecutorTest.cs +++ b/test/Microsoft.AspNet.Mvc.ViewFeatures.Test/ViewFeatures/ViewExecutorTest.cs @@ -309,6 +309,8 @@ namespace Microsoft.AspNet.Mvc.ViewFeatures await v.Writer.WriteAsync(text); }); + var expectedWriteCallCount = Math.Ceiling((double)writeLength / HttpResponseStreamWriter.DefaultBufferSize); + var context = new DefaultHttpContext(); var stream = new Mock(); stream.SetupGet(s => s.CanWrite).Returns(true); @@ -332,7 +334,10 @@ namespace Microsoft.AspNet.Mvc.ViewFeatures statusCode: null); // Assert - stream.Verify(s => s.FlushAsync(It.IsAny()), Times.Once()); + stream.Verify(s => s.FlushAsync(It.IsAny()), Times.Never()); + stream.Verify( + s => s.WriteAsync(It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny()), + Times.Exactly((int)expectedWriteCallCount)); stream.Verify(s => s.Write(It.IsAny(), It.IsAny(), It.IsAny()), Times.Never()); }