DevDiv 388456 -- WebHost should not use Transfer-Encoding chunked when content length is known

roncain · roncain · commit 06f52b894414 · 2012-04-03T07:12:23.000-07:00
diff --git a/src/System.Web.Http.WebHost/HttpControllerHandler.cs b/src/System.Web.Http.WebHost/HttpControllerHandler.cs
@@ -278,12 +278,19 @@ internal static Task ConvertResponse(HttpContextBase httpContextBase, HttpRespon
 
             if (response.Content != null)
             {
-                CopyHeaders(response.Content.Headers, httpContextBase);
-
-                // Select output buffering by the kind of content
+                // Select output buffering based on the kind of HttpContent.  
+                // This is done before CopyHeaders because the ContentLength
+                // property getter will evaluate the content length and set
+                // the Content-Length header if it has not already been set.
+                // Doing this before CopyHeaders ensures the headers contain a 
+                // valid Content-Length before they are copied to HttpContextBase.
+                // Unless HttpContextBase headers contain a positive Content-Length,
+                // the Transfer-Encoding for streamed output will be chunked.
                 isBuffered = IsOutputBufferingNecessary(response.Content);
                 httpResponseBase.BufferOutput = isBuffered;
 
+                CopyHeaders(response.Content.Headers, httpContextBase);
+
                 responseTask = response.Content.CopyToAsync(httpResponseBase.OutputStream);
             }
             else
@@ -328,24 +335,24 @@ internal static Task ConvertResponse(HttpContextBase httpContextBase, HttpRespon
                 });
         }
 
-        private static bool IsOutputBufferingNecessary(HttpContent httpContent)
+        /// <summary>
+        /// Determines whether the given <see cref="HttpContent"/> should use a buffered response.
+        /// </summary>
+        /// <param name="httpContent">The <see cref="HttpContent"/> of the response.</param>
+        /// <returns>A value of <c>true</c> indicates buffering should be used, otherwise a streamed response should be used.</returns>
+        internal static bool IsOutputBufferingNecessary(HttpContent httpContent)
         {
-            // Never buffer StreamContent.  Either it's already buffered or
-            // is of indeterminate length.  Neither calls for us to buffer.
-            if (httpContent == null || httpContent is StreamContent)
-            {
-                return false;
-            }
+            Contract.Assert(httpContent != null);
 
-            // Any content that knows its ContentLength is assumed to have
-            // buffered it already.
+            // Any HttpContent that knows its length is presumably already buffered internally.
             long? contentLength = httpContent.Headers.ContentLength;
             if (contentLength.HasValue && contentLength.Value >= 0)
             {
                 return false;
             }
 
-            return true;
+            // Content length is null or -1 (meaning not known).  Buffer any HttpContent except StreamContent.
+            return !(httpContent is StreamContent);
         }
 
         [SuppressMessage("Microsoft.Reliability", "CA2000:Dispose objects before losing scope", Justification = "Caller becomes owner")]
diff --git a/test/System.Web.Http.WebHost.Test/HttpControllerHandlerTest.cs b/test/System.Web.Http.WebHost.Test/HttpControllerHandlerTest.cs
@@ -1,12 +1,18 @@
 ﻿using System.Collections;
+using System.Collections.Generic;
 using System.IO;
+using System.Linq;
 using System.Net;
 using System.Net.Http;
+using System.Net.Http.Formatting;
 using System.Net.Http.Headers;
 using System.Reflection;
+using System.Text;
 using System.Threading.Tasks;
+using Microsoft.TestCommon;
 using Moq;
 using Xunit;
+using Xunit.Extensions;
 using Assert = Microsoft.TestCommon.AssertEx;
 
 namespace System.Web.Http.WebHost
@@ -133,5 +139,56 @@ public void SuppressFormsAuthenticationRedirect_RequireSuppressRedirect() {
             }
         }
 
+        [Theory]
+        [PropertyData("OutputBufferingTestData")]
+        public void IsOutputBufferingNecessary_Returns_Correct_Value(HttpContent content, bool isBuffered)
+        {
+            // Arrange & Act & Assert
+            Assert.Equal(isBuffered, HttpControllerHandler.IsOutputBufferingNecessary(content));
+        }
+
+        [Theory]
+        [PropertyData("OutputBufferingTestData")]
+        public void IsOutputBufferingNecessary_Causes_Content_Length_Header_To_Be_Set(HttpContent content, bool isBuffered)
+        {
+            // Arrange & Act
+            HttpControllerHandler.IsOutputBufferingNecessary(content);
+            IEnumerable<string> contentLengthEnumerable;
+            bool isContentLengthInHeaders = content.Headers.TryGetValues("Content-Length", out contentLengthEnumerable);
+            string[] contentLengthStrings = isContentLengthInHeaders ? contentLengthEnumerable.ToArray() : new string[0];
+            long? contentLength = content.Headers.ContentLength;
+
+            // Assert
+            if (contentLength.HasValue && contentLength.Value >= 0)
+            {
+                // Setting the header is HttpContentHeader's responsibility, but we assert
+                // it has happened because it is IsOutputBufferingNecessary's responsibility
+                // to cause that to happen.   HttpControllerHandler relies on this.
+                Assert.True(isContentLengthInHeaders);
+                Assert.Equal(contentLength.Value, long.Parse(contentLengthStrings[0]));
+            }
+        }
+
+        public static IEnumerable<object[]> OutputBufferingTestData
+        {
+            get
+            {
+                string testString = "testString";
+                Mock<Stream> mockStream = new Mock<Stream>() { CallBase = true };
+                return new TheoryDataSet<HttpContent, bool>()
+                {
+                    // Known length HttpContents other than OC should not buffer
+                    { new StringContent(testString), false },
+                    { new ByteArrayContent(Encoding.UTF8.GetBytes(testString)), false },
+                    { new StreamContent(new MemoryStream(Encoding.UTF8.GetBytes(testString))), false},
+
+                    // StreamContent (unknown length) should not buffer
+                    { new StreamContent(mockStream.Object), false},
+
+                    // ObjectContent (unknown length) should buffer
+                    { new ObjectContent<string>(testString, new XmlMediaTypeFormatter()), true }
+                };
+            }
+        }
     }
 }
diff --git a/test/System.Web.Http.WebHost.Test/System.Web.Http.WebHost.Test.csproj b/test/System.Web.Http.WebHost.Test/System.Web.Http.WebHost.Test.csproj
@@ -72,6 +72,10 @@
     <None Include="packages.config" />
   </ItemGroup>
   <ItemGroup>
+    <ProjectReference Include="..\..\src\System.Net.Http.Formatting\System.Net.Http.Formatting.csproj">
+      <Project>{668E9021-CE84-49D9-98FB-DF125A9FCDB0}</Project>
+      <Name>System.Net.Http.Formatting</Name>
+    </ProjectReference>
     <ProjectReference Include="..\..\src\System.Web.Http.WebHost\System.Web.Http.WebHost.csproj">
       <Project>{A0187BC2-8325-4BB2-8697-7F955CF4173E}</Project>
       <Name>System.Web.Http.WebHost</Name>
