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.

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)

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();
+            }
         }
     }
 }

src/Microsoft.AspNet.Mvc.Formatters.Xml/XmlDataContractSerializerOutputFormatter.cs

@@ -179,7 +179,7 @@ namespace Microsoft.AspNet.Mvc.Formatters
         }
 
         /// <inheritdoc />
-        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();
+            }
         }
 
         /// <summary>

src/Microsoft.AspNet.Mvc.Formatters.Xml/XmlSerializerOutputFormatter.cs

@@ -154,7 +154,7 @@ namespace Microsoft.AspNet.Mvc.Formatters
         }
 
         /// <inheritdoc />
-        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();
+            }
         }
 
         /// <summary>

src/Microsoft.AspNet.Mvc.Razor/RazorPage.cs

@@ -812,8 +812,8 @@ namespace Microsoft.AspNet.Mvc.Razor
         }
 
         /// <summary>
-        /// Invokes <see cref="TextWriter.FlushAsync"/> on <see cref="Output"/> writing out any buffered
+        /// Invokes <see cref="TextWriter.FlushAsync"/> on <see cref="Output"/> and <see cref="Stream.FlushAsync"/>
-        /// content to the <see cref="HttpResponse.Body"/>.
+        /// on the response stream, writing out any buffered content to the <see cref="HttpResponse.Body"/>.
         /// </summary>
         /// <returns>A <see cref="Task{HtmlString}"/> that represents the asynchronous flush operation and on
         /// completion returns <see cref="HtmlString.Empty"/>.</returns>
@@ -842,6 +842,7 @@ namespace Microsoft.AspNet.Mvc.Razor
             }
 
             await Output.FlushAsync();
+            await Context.Response.Body.FlushAsync();
             return HtmlString.Empty;
         }
 

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;
+            public int FlushCallCount { get; private set; }
-            private int _flushAsyncCallCount;
-            private int _disposeCallCount;
 
-            public int FlushCallCount { get { return _flushCallCount; } }
+            public int FlushAsyncCallCount { get; private set; }
-
-            public int FlushAsyncCallCount { get { return _flushAsyncCallCount; } }
 
             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);
             }
         }

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>();
             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<CancellationToken>()), Times.Once());
+            stream.Verify(s => s.FlushAsync(It.IsAny<CancellationToken>()), Times.Never());
+            stream.Verify(
+                s => s.WriteAsync(It.IsAny<byte[]>(), It.IsAny<int>(), It.IsAny<int>(), It.IsAny<CancellationToken>()), 
+                Times.Exactly((int)expectedWriteCallCount));
             stream.Verify(s => s.Write(It.IsAny<byte[]>(), It.IsAny<int>(), It.IsAny<int>()), Times.Never());
         }