Fix HttpContent.ReadAsStream length, don't buffer StreamContent unless necessary (#416)

Author: patagonaa

Tests/HttpUnitTests/StreamContentTest.cs

@@ -176,6 +176,65 @@ public void CopyTo_CallMultipleTimesWithStreamNotSupportingSeekingButBufferedStr
             Assert.Equal(10 - consumed, destination2.Length);
         }
 
+        [TestMethod]
+        public void CopyTo_NoLoadIntoBuffer_NotBuffered()
+        {
+            var initialSourceContent = new byte[10] { 0, 1, 2, 3, 4, 5, 6, 7, 8, 9 };
+
+            var sourceContent = new byte[10];
+            Array.Copy(initialSourceContent, sourceContent, 10);
+
+            var sourceStream = new MockStream(sourceContent, true, true);
+
+            var content = new StreamContent(sourceStream);
+
+            var destination1 = new MemoryStream();
+            content.CopyTo(destination1);
+
+            Assert.AreEqual(10, destination1.Length);
+            CollectionAssert.AreEqual(initialSourceContent, destination1.ToArray());
+
+            // replace source content to ensure data was not buffered in the content
+            var replacedSourceContent = new byte[10] { 9, 8, 7, 6, 5, 4, 3, 2, 1, 0 };
+            Array.Copy(replacedSourceContent, sourceContent, 10);
+
+            var destination2 = new MemoryStream();
+            content.CopyTo(destination2);
+            Assert.AreEqual(10, destination2.Length);
+            CollectionAssert.AreEqual(replacedSourceContent, destination2.ToArray());
+        }
+
+        [TestMethod]
+        public void CopyTo_LoadIntoBuffer_Buffered()
+        {
+            var initialSourceContent = new byte[10] { 0, 1, 2, 3, 4, 5, 6, 7, 8, 9 };
+
+            var sourceContent = new byte[10];
+            Array.Copy(initialSourceContent, sourceContent, 10);
+
+            var sourceStream = new MockStream(sourceContent, true, true);
+
+            var content = new StreamContent(sourceStream);
+
+            // buffer so changing the source stream doesn't change the Content
+            content.LoadIntoBuffer();
+
+            var destination1 = new MemoryStream();
+            content.CopyTo(destination1);
+
+            Assert.AreEqual(10, destination1.Length);
+            CollectionAssert.AreEqual(initialSourceContent, destination1.ToArray());
+
+            // replace source content to ensure data was buffered in the content
+            var replacedSourceContent = new byte[10] { 9, 8, 7, 6, 5, 4, 3, 2, 1, 0 };
+            Array.Copy(replacedSourceContent, sourceContent, 10);
+
+            var destination2 = new MemoryStream();
+            content.CopyTo(destination2);
+            Assert.AreEqual(10, destination2.Length);
+            CollectionAssert.AreEqual(initialSourceContent, destination2.ToArray());
+        }
+
         [TestMethod]
         public void ContentReadStream_GetProperty_ReturnOriginalStream()
         {
@@ -189,6 +248,20 @@ public void ContentReadStream_GetProperty_ReturnOriginalStream()
             Assert.NotSame(source, stream);
         }
 
+        [TestMethod]
+        public void ContentReadStream_GetProperty_LoadIntoBuffer_ReturnOriginalStream()
+        {
+            var source = new MockStream(new byte[10]);
+            var content = new StreamContent(source);
+            content.LoadIntoBuffer();
+
+            Stream stream = content.ReadAsStream();
+            Assert.IsFalse(stream.CanWrite);
+            Assert.AreEqual(source.Length, stream.Length);
+            Assert.AreEqual(0, source.ReadCount);
+            Assert.AreNotSame(source, stream);
+        }
+
         [TestMethod]
         public void ContentReadStream_GetPropertyPartiallyConsumed_ReturnOriginalStream()
         {
@@ -220,7 +293,7 @@ public void ContentReadStream_CheckResultProperties_ValuesRepresentReadOnlyStrea
             var content = new StreamContent(source);
             Stream contentReadStream = content.ReadAsStream();
 
-            // The following checks verify that the stream returned passes all read-related properties to the 
+            // The following checks verify that the stream returned passes all read-related properties to the
             // underlying MockStream and throws when using write-related members.
 
             Assert.False(contentReadStream.CanWrite);

nanoFramework.System.Net.Http/Http/HttpClient.cs

@@ -309,7 +309,9 @@ public byte[] GetByteArray(string requestUri)
         /// This operation will block.
         /// </para>
         /// <para>
-        /// This method does not read nor buffer the response body. It will return as soon as the response headers are read.
+        /// As of now, this method reads and buffers the entire response body (see <see cref="StreamContent.SerializeToStream(Stream)"/>).
+        /// If the response needs to be streamed, one of the methods that return <see cref="Send(HttpRequestMessage, HttpCompletionOption)"/> should be used
+        /// with <see cref="HttpCompletionOption.ResponseHeadersRead"/> and <see cref="HttpContent.CopyTo(Stream)"/> instead.
         /// </para>
         /// <para>
         /// This is the .NET nanoFramework equivalent of GetStreamAsync.
@@ -434,7 +436,7 @@ private HttpResponseMessage SendWorker(HttpRequestMessage request, HttpCompletio
             HttpResponseMessage response = base.Send(request);
 
             // Read the content when default HttpCompletionOption.ResponseContentRead is set
-            if (response.Content != null && (completionOption & HttpCompletionOption.ResponseHeadersRead) == 0)
+            if (response.Content != null && completionOption == HttpCompletionOption.ResponseContentRead)
             {
                 response.Content.LoadIntoBuffer();
             }

nanoFramework.System.Net.Http/Http/HttpClientHandler.cs

@@ -224,22 +224,26 @@ protected internal override HttpResponseMessage Send(HttpRequestMessage request)
                         headers.AddInternal(headerKey, content.Headers._headerStore[headerKey]);
                     }
 
-                    if (request.Headers.TransferEncodingChunked == true)
+                    if (request.Headers.TransferEncodingChunked)
                     {
                         webRequest.SendChunked = true;
                     }
 
                     // set content length
-                    request.Content.TryComputeLength(out long lenght);
-                    webRequest.ContentLength = lenght;
+                    // if it can't be computed and Transfer-Encoding: chunked isn't set,
+                    // webRequest.GetRequestStream throws an exception (so we don't have to validate that here).
+                    if (request.Content.TryComputeLength(out long length))
+                    {
+                        webRequest.ContentLength = length;
+                    }
+
 
                     // set request sent flag
                     _sentRequest = true;
 
                     var stream = webRequest.GetRequestStream();
 
-                    request.Content.ReadAsStream().CopyTo(stream);
-
+                    request.Content.CopyTo(stream);
                 }
                 else if (MethodHasBody(request.Method))
                 {

nanoFramework.System.Net.Http/Http/HttpContent.cs

@@ -16,12 +16,10 @@ namespace System.Net.Http
     public abstract class HttpContent : IDisposable
     {
         private HttpContentHeaders _headers;
-        private FixedMemoryStream _buffer;
-        private Stream _stream;
+        private MemoryStream _buffer;
 
         private bool _disposed;
 
-        internal const int MaxBufferSize = int.MaxValue;
         internal static readonly Encoding DefaultStringEncoding = Encoding.UTF8;
 
         /// <summary>
@@ -41,11 +39,6 @@ public HttpContentHeaders Headers
             }
         }
 
-        /// <summary>
-        /// Contains the actual bytes read into the buffer
-        /// </summary>
-        protected int TotalBytesRead { get; set; }
-
         /// <summary>
         /// Initializes a new instance of the HttpContent class.
         /// </summary>
@@ -72,7 +65,9 @@ public void CopyTo(Stream stream)
 
             if (_buffer != null)
             {
+                _buffer.Position = 0;
                 _buffer.CopyTo(stream);
+                return;
             }
 
             SerializeToStream(stream);
@@ -94,7 +89,7 @@ public void CopyTo(Stream stream)
         /// </para>
         /// </remarks>
         /// <exception cref="ObjectDisposedException">If the object has been disposed.</exception>
-        public Stream LoadIntoBuffer()
+        public void LoadIntoBuffer()
         {
             if (_disposed)
             {
@@ -103,21 +98,12 @@ public Stream LoadIntoBuffer()
 
             if (_buffer != null)
             {
-                return _buffer;
-            }
-
-            _buffer = new FixedMemoryStream(int.MaxValue);
-
-            SerializeToStream(_buffer);
-
-            if (TotalBytesRead > 0)
-            {
-                _buffer.SetLength(TotalBytesRead);
+                return;
             }
 
-            _buffer.Seek(0, SeekOrigin.Begin);
-
-            return _buffer;
+            var buffer = new MemoryStream();
+            SerializeToStream(buffer);
+            _buffer = buffer;
         }
 
         /// <summary>
@@ -127,24 +113,7 @@ public Stream LoadIntoBuffer()
         /// <exception cref="ObjectDisposedException">If the object has been disposed.</exception>
         public Stream ReadAsStream()
         {
-            if (_disposed)
-            {
-                throw new ObjectDisposedException();
-            }
-
-            if (_buffer != null)
-            {
-
-                return new MemoryStream(_buffer.GetBuffer());
-            }
-
-            if (_stream == null)
-            {
-
-                _stream = LoadIntoBuffer();
-            }
-
-            return _stream;
+            return CreateContentReadStream();
         }
 
         /// <summary>
@@ -237,35 +206,22 @@ protected virtual void Dispose(bool disposing)
         /// </remarks>
         protected abstract void SerializeToStream(Stream stream);
 
-        private sealed class FixedMemoryStream : MemoryStream
+        /// <summary>
+        /// If overridden, returns the HTTP content as a stream.
+        /// Otherwise, <see cref="SerializeToStream(Stream)"/> is called.
+        /// </summary>
+        /// <returns></returns>
+        /// <remarks>
+        /// <para>
+        /// This is the .NET nanoFramework equivalent of CreateContentReadStreamAsync().
+        /// </para>
+        /// </remarks>
+        protected virtual Stream CreateContentReadStream()
         {
-            readonly long _maxSize;
-
-            public FixedMemoryStream(long maxSize)
-                : base()
-            {
-                _maxSize = maxSize;
-            }
-
-            private void CheckOverflow(int count)
-            {
-                if (Length + count > _maxSize)
-                {
-                    throw new HttpRequestException();
-                }
-            }
-
-            public override void WriteByte(byte value)
-            {
-                CheckOverflow(1);
-                base.WriteByte(value);
-            }
+            LoadIntoBuffer();
 
-            public override void Write(byte[] buffer, int offset, int count)
-            {
-                CheckOverflow(count);
-                base.Write(buffer, offset, count);
-            }
+            _buffer.Seek(0, SeekOrigin.Begin);
+            return _buffer;
         }
     }
 }