Avoid buffering during input formatting for longer than necessary (#9806)

* Avoid buffering during input formatting for longer than necessary

EnableRewind uses FileBufferingReadStream which is not disposed until the response is completed.
This results in holding on to internal buffers for significantly longer than necessary. Changing it
to  return the buffers immediately improved the allocations and throughput.

src/Http/WebUtilities/ref/Microsoft.AspNetCore.WebUtilities.netcoreapp3.0.cs

@@ -41,6 +41,7 @@ namespace Microsoft.AspNetCore.WebUtilities
     }
     public partial class FileBufferingReadStream : System.IO.Stream
     {
+        public FileBufferingReadStream(System.IO.Stream inner, int memoryThreshold) { }
         public FileBufferingReadStream(System.IO.Stream inner, int memoryThreshold, long? bufferLimit, System.Func<string> tempFileDirectoryAccessor) { }
         public FileBufferingReadStream(System.IO.Stream inner, int memoryThreshold, long? bufferLimit, System.Func<string> tempFileDirectoryAccessor, System.Buffers.ArrayPool<byte> bytePool) { }
         public FileBufferingReadStream(System.IO.Stream inner, int memoryThreshold, long? bufferLimit, string tempFileDirectory) { }

src/Http/WebUtilities/src/FileBufferingReadStream.cs

@@ -7,6 +7,7 @@ using System.Diagnostics;
 using System.IO;
 using System.Threading;
 using System.Threading.Tasks;
+using Microsoft.AspNetCore.Internal;
 
 namespace Microsoft.AspNetCore.WebUtilities
 {
@@ -33,6 +34,16 @@ namespace Microsoft.AspNetCore.WebUtilities
 
         private bool _disposed;
 
+        /// <summary>
+        /// Initializes a new instance of <see cref="FileBufferingReadStream" />.
+        /// </summary>
+        /// <param name="inner">The wrapping <see cref="Stream" />.</param>
+        /// <param name="memoryThreshold">The maximum size to buffer in memory.</param>
+        public FileBufferingReadStream(Stream inner, int memoryThreshold)
+            : this(inner, memoryThreshold, bufferLimit: null, tempFileDirectoryAccessor: AspNetCoreTempDirectory.TempDirectoryFactory)
+        {
+        }
+
         public FileBufferingReadStream(
             Stream inner,
             int memoryThreshold,
@@ -223,13 +234,19 @@ namespace Microsoft.AspNetCore.WebUtilities
                 {
                     oldBuffer.Position = 0;
                     var rentedBuffer = _bytePool.Rent(Math.Min((int)oldBuffer.Length, _maxRentedBufferSize));
-                    var copyRead = oldBuffer.Read(rentedBuffer, 0, rentedBuffer.Length);
-                    while (copyRead > 0)
+                    try
                     {
-                        _buffer.Write(rentedBuffer, 0, copyRead);
-                        copyRead = oldBuffer.Read(rentedBuffer, 0, rentedBuffer.Length);
+                        var copyRead = oldBuffer.Read(rentedBuffer, 0, rentedBuffer.Length);
+                        while (copyRead > 0)
+                        {
+                            _buffer.Write(rentedBuffer, 0, copyRead);
+                            copyRead = oldBuffer.Read(rentedBuffer, 0, rentedBuffer.Length);
+                        }
+                    }
+                    finally
+                    {
+                        _bytePool.Return(rentedBuffer);
                     }
-                    _bytePool.Return(rentedBuffer);
                 }
                 else
                 {
@@ -277,14 +294,20 @@ namespace Microsoft.AspNetCore.WebUtilities
                 {
                     oldBuffer.Position = 0;
                     var rentedBuffer = _bytePool.Rent(Math.Min((int)oldBuffer.Length, _maxRentedBufferSize));
-                    // oldBuffer is a MemoryStream, no need to do async reads.
-                    var copyRead = oldBuffer.Read(rentedBuffer, 0, rentedBuffer.Length);
-                    while (copyRead > 0)
+                    try
                     {
-                        await _buffer.WriteAsync(rentedBuffer, 0, copyRead, cancellationToken);
-                        copyRead = oldBuffer.Read(rentedBuffer, 0, rentedBuffer.Length);
+                        // oldBuffer is a MemoryStream, no need to do async reads.
+                        var copyRead = oldBuffer.Read(rentedBuffer, 0, rentedBuffer.Length);
+                        while (copyRead > 0)
+                        {
+                            await _buffer.WriteAsync(rentedBuffer, 0, copyRead, cancellationToken);
+                            copyRead = oldBuffer.Read(rentedBuffer, 0, rentedBuffer.Length);
+                        }
+                    }
+                    finally
+                    {
+                        _bytePool.Return(rentedBuffer);
                     }
-                    _bytePool.Return(rentedBuffer);
                 }
                 else
                 {
@@ -351,4 +374,4 @@ namespace Microsoft.AspNetCore.WebUtilities
             }
         }
     }
-}
+}

src/Http/WebUtilities/test/FileBufferingReadStreamTests.cs

@@ -2,9 +2,11 @@
 // 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;
 using System.Threading.Tasks;
+using Moq;
 using Xunit;
 
 namespace Microsoft.AspNetCore.WebUtilities
@@ -291,9 +293,67 @@ namespace Microsoft.AspNetCore.WebUtilities
             Assert.False(File.Exists(tempFileName));
         }
 
+        [Fact]
+        public void FileBufferingReadStream_UsingMemoryStream_RentsAndReturnsRentedBuffer_WhenCopyingFromMemoryStreamDuringRead()
+        {
+            var inner = MakeStream(1024 * 1024 + 25);
+            string tempFileName;
+            var arrayPool = new Mock<ArrayPool<byte>>();
+            arrayPool.Setup(p => p.Rent(It.IsAny<int>()))
+                .Returns((int m) => ArrayPool<byte>.Shared.Rent(m));
+            arrayPool.Setup(p => p.Return(It.IsAny<byte[]>(), It.IsAny<bool>()))
+                .Callback((byte[] bytes, bool clear) => ArrayPool<byte>.Shared.Return(bytes, clear));
+
+            using (var stream = new FileBufferingReadStream(inner, 1024 * 1024 + 1, 2 * 1024 * 1024, GetCurrentDirectory(), arrayPool.Object))
+            {
+                arrayPool.Verify(v => v.Rent(It.IsAny<int>()), Times.Never());
+
+                stream.Read(new byte[1024 * 1024]);
+                Assert.False(File.Exists(stream.TempFileName), "tempFile should not be created as yet");
+
+                stream.Read(new byte[4]);
+                Assert.True(File.Exists(stream.TempFileName), "tempFile should be created");
+                tempFileName = stream.TempFileName;
+
+                arrayPool.Verify(v => v.Rent(It.IsAny<int>()), Times.Once());
+                arrayPool.Verify(v => v.Return(It.IsAny<byte[]>(), It.IsAny<bool>()), Times.Once());
+            }
+
+            Assert.False(File.Exists(tempFileName));
+        }
+
+        [Fact]
+        public async Task FileBufferingReadStream_UsingMemoryStream_RentsAndReturnsRentedBuffer_WhenCopyingFromMemoryStreamDuringReadAsync()
+        {
+            var inner = MakeStream(1024 * 1024 + 25);
+            string tempFileName;
+            var arrayPool = new Mock<ArrayPool<byte>>();
+            arrayPool.Setup(p => p.Rent(It.IsAny<int>()))
+                .Returns((int m) => ArrayPool<byte>.Shared.Rent(m));
+            arrayPool.Setup(p => p.Return(It.IsAny<byte[]>(), It.IsAny<bool>()))
+                .Callback((byte[] bytes, bool clear) => ArrayPool<byte>.Shared.Return(bytes, clear));
+
+            using (var stream = new FileBufferingReadStream(inner, 1024 * 1024 + 1, 2 * 1024 * 1024, GetCurrentDirectory(), arrayPool.Object))
+            {
+                arrayPool.Verify(v => v.Rent(It.IsAny<int>()), Times.Never());
+
+                await stream.ReadAsync(new byte[1024 * 1024]);
+                Assert.False(File.Exists(stream.TempFileName), "tempFile should not be created as yet");
+
+                await stream.ReadAsync(new byte[4]);
+                Assert.True(File.Exists(stream.TempFileName), "tempFile should be created");
+                tempFileName = stream.TempFileName;
+
+                arrayPool.Verify(v => v.Rent(It.IsAny<int>()), Times.Once());
+                arrayPool.Verify(v => v.Return(It.IsAny<byte[]>(), It.IsAny<bool>()), Times.Once());
+            }
+
+            Assert.False(File.Exists(tempFileName));
+        }
+
         private static string GetCurrentDirectory()
         {
             return AppContext.BaseDirectory;
         }
     }
-}
+}

src/Mvc/Mvc.Core/src/MvcOptions.cs

@@ -103,7 +103,8 @@ namespace Microsoft.AspNetCore.Mvc
         public FormatterCollection<IInputFormatter> InputFormatters { get; }
 
         /// <summary>
-        /// Gets or sets the flag to buffer the request body in input formatters. Default is <c>false</c>.
+        /// Gets or sets a value that determines if buffering is disabled for input formatters that
+        /// synchronously read from the HTTP request body.
         /// </summary>
         public bool SuppressInputFormatterBuffering { get; set; }
 

src/Mvc/Mvc.Formatters.Xml/src/XmlDataContractSerializerInputFormatter.cs

@@ -4,14 +4,12 @@
 using System;
 using System.Collections.Concurrent;
 using System.Collections.Generic;
-using System.Diagnostics;
 using System.IO;
 using System.Runtime.Serialization;
 using System.Text;
 using System.Threading;
 using System.Threading.Tasks;
 using System.Xml;
-using Microsoft.AspNetCore.Http;
 using Microsoft.AspNetCore.Mvc.Formatters.Xml;
 using Microsoft.AspNetCore.Mvc.Infrastructure;
 using Microsoft.AspNetCore.WebUtilities;
@@ -24,6 +22,7 @@ namespace Microsoft.AspNetCore.Mvc.Formatters
     /// </summary>
     public class XmlDataContractSerializerInputFormatter : TextInputFormatter, IInputFormatterExceptionPolicy
     {
+        private const int DefaultMemoryThreshold = 1024 * 30;
         private readonly ConcurrentDictionary<Type, object> _serializerCache = new ConcurrentDictionary<Type, object>();
         private readonly XmlDictionaryReaderQuotas _readerQuotas = FormattingUtilities.GetDefaultXmlReaderQuotas();
         private readonly MvcOptions _options;
@@ -118,43 +117,55 @@ namespace Microsoft.AspNetCore.Mvc.Formatters
             }
 
             var request = context.HttpContext.Request;
+            Stream readStream = new NonDisposableStream(request.Body);
 
             if (!request.Body.CanSeek && !_options.SuppressInputFormatterBuffering)
             {
                 // XmlDataContractSerializer does synchronous reads. In order to avoid blocking on the stream, we asynchronously
                 // read everything into a buffer, and then seek back to the beginning.
-                request.EnableBuffering();
-                Debug.Assert(request.Body.CanSeek);
+                var memoryThreshold = DefaultMemoryThreshold;
+                if (request.ContentLength.HasValue && request.ContentLength.Value > 0 && request.ContentLength.Value < memoryThreshold)
+                {
+                    // If the Content-Length is known and is smaller than the default buffer size, use it.
+                    memoryThreshold = (int)request.ContentLength.Value;
+                }
 
-                await request.Body.DrainAsync(CancellationToken.None);
-                request.Body.Seek(0L, SeekOrigin.Begin);
+                readStream = new FileBufferingReadStream(request.Body, memoryThreshold);
+
+                await readStream.DrainAsync(CancellationToken.None);
+                readStream.Seek(0L, SeekOrigin.Begin);
             }
 
             try
             {
-                using (var xmlReader = CreateXmlReader(new NonDisposableStream(request.Body), encoding))
+                using var xmlReader = CreateXmlReader(readStream, encoding);
+                var type = GetSerializableType(context.ModelType);
+                var serializer = GetCachedSerializer(type);
+
+                var deserializedObject = serializer.ReadObject(xmlReader);
+
+                // Unwrap only if the original type was wrapped.
+                if (type != context.ModelType)
                 {
-                    var type = GetSerializableType(context.ModelType);
-                    var serializer = GetCachedSerializer(type);
-
-                    var deserializedObject = serializer.ReadObject(xmlReader);
-
-                    // Unwrap only if the original type was wrapped.
-                    if (type != context.ModelType)
+                    if (deserializedObject is IUnwrappable unwrappable)
                     {
-                        if (deserializedObject is IUnwrappable unwrappable)
-                        {
-                            deserializedObject = unwrappable.Unwrap(declaredType: context.ModelType);
-                        }
+                        deserializedObject = unwrappable.Unwrap(declaredType: context.ModelType);
                     }
-
-                    return InputFormatterResult.Success(deserializedObject);
                 }
+
+                return InputFormatterResult.Success(deserializedObject);
             }
             catch (SerializationException exception)
             {
                 throw new InputFormatterException(Resources.ErrorDeserializingInputData, exception);
             }
+            finally
+            {
+                if (readStream is FileBufferingReadStream fileBufferingReadStream)
+                {
+                    fileBufferingReadStream.Dispose();
+                }
+            }
         }
 
         /// <inheritdoc />

src/Mvc/Mvc.Formatters.Xml/src/XmlSerializerInputFormatter.cs

@@ -4,14 +4,12 @@
 using System;
 using System.Collections.Concurrent;
 using System.Collections.Generic;
-using System.Diagnostics;
 using System.IO;
 using System.Text;
 using System.Threading;
 using System.Threading.Tasks;
 using System.Xml;
 using System.Xml.Serialization;
-using Microsoft.AspNetCore.Http;
 using Microsoft.AspNetCore.Mvc.Formatters.Xml;
 using Microsoft.AspNetCore.Mvc.Infrastructure;
 using Microsoft.AspNetCore.WebUtilities;
@@ -24,6 +22,7 @@ namespace Microsoft.AspNetCore.Mvc.Formatters
     /// </summary>
     public class XmlSerializerInputFormatter : TextInputFormatter, IInputFormatterExceptionPolicy
     {
+        private const int DefaultMemoryThreshold = 1024 * 30;
         private readonly ConcurrentDictionary<Type, object> _serializerCache = new ConcurrentDictionary<Type, object>();
         private readonly XmlDictionaryReaderQuotas _readerQuotas = FormattingUtilities.GetDefaultXmlReaderQuotas();
         private readonly MvcOptions _options;
@@ -99,39 +98,43 @@ namespace Microsoft.AspNetCore.Mvc.Formatters
             }
 
             var request = context.HttpContext.Request;
-
+            Stream readStream = new NonDisposableStream(request.Body);
             if (!request.Body.CanSeek && !_options.SuppressInputFormatterBuffering)
             {
                 // XmlSerializer does synchronous reads. In order to avoid blocking on the stream, we asynchronously
                 // read everything into a buffer, and then seek back to the beginning.
-                request.EnableBuffering();
-                Debug.Assert(request.Body.CanSeek);
+                var memoryThreshold = DefaultMemoryThreshold;
+                if (request.ContentLength.HasValue && request.ContentLength.Value > 0 && request.ContentLength.Value < memoryThreshold)
+                {
+                    // If the Content-Length is known and is smaller than the default buffer size, use it.
+                    memoryThreshold = (int)request.ContentLength.Value;
+                }
 
-                await request.Body.DrainAsync(CancellationToken.None);
-                request.Body.Seek(0L, SeekOrigin.Begin);
+                readStream = new FileBufferingReadStream(request.Body, memoryThreshold);
+
+                await readStream.DrainAsync(CancellationToken.None);
+                readStream.Seek(0L, SeekOrigin.Begin);
             }
 
             try
             {
-                using (var xmlReader = CreateXmlReader(new NonDisposableStream(request.Body), encoding))
+                using var xmlReader = CreateXmlReader(readStream, encoding);
+                var type = GetSerializableType(context.ModelType);
+
+                var serializer = GetCachedSerializer(type);
+
+                var deserializedObject = serializer.Deserialize(xmlReader);
+
+                // Unwrap only if the original type was wrapped.
+                if (type != context.ModelType)
                 {
-                    var type = GetSerializableType(context.ModelType);
-
-                    var serializer = GetCachedSerializer(type);
-
-                    var deserializedObject = serializer.Deserialize(xmlReader);
-
-                    // Unwrap only if the original type was wrapped.
-                    if (type != context.ModelType)
+                    if (deserializedObject is IUnwrappable unwrappable)
                     {
-                        if (deserializedObject is IUnwrappable unwrappable)
-                        {
-                            deserializedObject = unwrappable.Unwrap(declaredType: context.ModelType);
-                        }
+                        deserializedObject = unwrappable.Unwrap(declaredType: context.ModelType);
                     }
-
-                    return InputFormatterResult.Success(deserializedObject);
                 }
+
+                return InputFormatterResult.Success(deserializedObject);
             }
             // XmlSerializer wraps actual exceptions (like FormatException or XmlException) into an InvalidOperationException
             // https://github.com/dotnet/corefx/blob/master/src/System.Private.Xml/src/System/Xml/Serialization/XmlSerializer.cs#L652
@@ -149,6 +152,13 @@ namespace Microsoft.AspNetCore.Mvc.Formatters
             {
                 throw new InputFormatterException(Resources.ErrorDeserializingInputData, exception.InnerException);
             }
+            finally
+            {
+                if (readStream is FileBufferingReadStream fileBufferingReadStream)
+                {
+                    fileBufferingReadStream.Dispose();
+                }
+            }
         }
 
         /// <inheritdoc />

src/Mvc/Mvc.Formatters.Xml/test/XmlDataContractSerializerInputFormatterTest.cs

@@ -6,6 +6,7 @@ using System.IO;
 using System.Linq;
 using System.Runtime.Serialization;
 using System.Text;
+using System.Threading;
 using System.Threading.Tasks;
 using System.Xml;
 using Microsoft.AspNetCore.Http;
@@ -149,7 +150,7 @@ namespace Microsoft.AspNetCore.Mvc.Formatters.Xml
             var contentBytes = Encoding.UTF8.GetBytes(input);
             var httpContext = new DefaultHttpContext();
             httpContext.Features.Set<IHttpResponseFeature>(new TestResponseFeature());
-            httpContext.Request.Body = new NonSeekableReadStream(contentBytes);
+            httpContext.Request.Body = new NonSeekableReadStream(contentBytes, allowSyncReads: true);
             httpContext.Request.ContentType = "application/json";
             var context = GetInputFormatterContext(httpContext, typeof(TestLevelOne));
 
@@ -163,19 +164,6 @@ namespace Microsoft.AspNetCore.Mvc.Formatters.Xml
 
             Assert.Equal(expectedInt, model.SampleInt);
             Assert.Equal(expectedString, model.sampleString);
-
-            Assert.True(httpContext.Request.Body.CanSeek);
-            httpContext.Request.Body.Seek(0L, SeekOrigin.Begin);
-
-            result = await formatter.ReadAsync(context);
-
-            // Assert
-            Assert.NotNull(result);
-            Assert.False(result.HasError);
-            model = Assert.IsType<TestLevelOne>(result.Model);
-
-            Assert.Equal(expectedInt, model.SampleInt);
-            Assert.Equal(expectedString, model.sampleString);
         }
 
         [Fact]
@@ -227,8 +215,9 @@ namespace Microsoft.AspNetCore.Mvc.Formatters.Xml
             var formatter = new XmlDataContractSerializerInputFormatter(new MvcOptions());
             var contentBytes = Encoding.UTF8.GetBytes(input);
             var httpContext = new DefaultHttpContext();
+
             httpContext.Features.Set<IHttpResponseFeature>(new TestResponseFeature());
-            httpContext.Request.Body = new NonSeekableReadStream(contentBytes);
+            httpContext.Request.Body = new NonSeekableReadStream(contentBytes, allowSyncReads: false);
             httpContext.Request.ContentType = "application/json";
             var context = GetInputFormatterContext(httpContext, typeof(TestLevelOne));
 
@@ -242,19 +231,6 @@ namespace Microsoft.AspNetCore.Mvc.Formatters.Xml
 
             Assert.Equal(expectedInt, model.SampleInt);
             Assert.Equal(expectedString, model.sampleString);
-
-            Assert.True(httpContext.Request.Body.CanSeek);
-            httpContext.Request.Body.Seek(0L, SeekOrigin.Begin);
-
-            result = await formatter.ReadAsync(context);
-
-            // Assert
-            Assert.NotNull(result);
-            Assert.False(result.HasError);
-            model = Assert.IsType<TestLevelOne>(result.Model);
-
-            Assert.Equal(expectedInt, model.SampleInt);
-            Assert.Equal(expectedString, model.sampleString);
         }
 
         [Fact]
@@ -287,9 +263,6 @@ namespace Microsoft.AspNetCore.Mvc.Formatters.Xml
 
             Assert.Equal(expectedInt, model.SampleInt);
             Assert.Equal(expectedString, model.sampleString);
-
-            // Reading again should fail as buffering request body is disabled
-            await Assert.ThrowsAsync<XmlException>(() => formatter.ReadAsync(context));
         }
 
         [Fact]

src/Mvc/Mvc.Formatters.Xml/test/XmlSerializerInputFormatterTest.cs

@@ -57,7 +57,7 @@ namespace Microsoft.AspNetCore.Mvc.Formatters.Xml
             var contentBytes = Encoding.UTF8.GetBytes(input);
             var httpContext = new DefaultHttpContext();
             httpContext.Features.Set<IHttpResponseFeature>(new TestResponseFeature());
-            httpContext.Request.Body = new NonSeekableReadStream(contentBytes);
+            httpContext.Request.Body = new NonSeekableReadStream(contentBytes, allowSyncReads: true);
             httpContext.Request.ContentType = "application/json";
             var context = GetInputFormatterContext(httpContext, typeof(TestLevelOne));
 
@@ -69,22 +69,6 @@ namespace Microsoft.AspNetCore.Mvc.Formatters.Xml
             Assert.False(result.HasError);
             var model = Assert.IsType<TestLevelOne>(result.Model);
 
-            Assert.Equal(expectedInt, model.SampleInt);
-            Assert.Equal(expectedString, model.sampleString);
-            Assert.Equal(
-                XmlConvert.ToDateTime(expectedDateTime, XmlDateTimeSerializationMode.Utc),
-                model.SampleDate);
-
-            Assert.True(httpContext.Request.Body.CanSeek);
-            httpContext.Request.Body.Seek(0L, SeekOrigin.Begin);
-
-            result = await formatter.ReadAsync(context);
-
-            // Assert
-            Assert.NotNull(result);
-            Assert.False(result.HasError);
-            model = Assert.IsType<TestLevelOne>(result.Model);
-
             Assert.Equal(expectedInt, model.SampleInt);
             Assert.Equal(expectedString, model.sampleString);
             Assert.Equal(
@@ -127,9 +111,6 @@ namespace Microsoft.AspNetCore.Mvc.Formatters.Xml
             Assert.Equal(
                 XmlConvert.ToDateTime(expectedDateTime, XmlDateTimeSerializationMode.Utc),
                 model.SampleDate);
-
-            // Reading again should fail as buffering request body is disabled
-            await Assert.ThrowsAsync<XmlException>(() => formatter.ReadAsync(context));
         }
 
         [Fact]
@@ -149,7 +130,7 @@ namespace Microsoft.AspNetCore.Mvc.Formatters.Xml
             var contentBytes = Encoding.UTF8.GetBytes(input);
             var httpContext = new DefaultHttpContext();
             httpContext.Features.Set<IHttpResponseFeature>(new TestResponseFeature());
-            httpContext.Request.Body = new NonSeekableReadStream(contentBytes);
+            httpContext.Request.Body = new NonSeekableReadStream(contentBytes, allowSyncReads: false);
             httpContext.Request.ContentType = "application/json";
             var context = GetInputFormatterContext(httpContext, typeof(TestLevelOne));
 
@@ -161,22 +142,6 @@ namespace Microsoft.AspNetCore.Mvc.Formatters.Xml
             Assert.False(result.HasError);
             var model = Assert.IsType<TestLevelOne>(result.Model);
 
-            Assert.Equal(expectedInt, model.SampleInt);
-            Assert.Equal(expectedString, model.sampleString);
-            Assert.Equal(
-                XmlConvert.ToDateTime(expectedDateTime, XmlDateTimeSerializationMode.Utc),
-                model.SampleDate);
-
-            Assert.True(httpContext.Request.Body.CanSeek);
-            httpContext.Request.Body.Seek(0L, SeekOrigin.Begin);
-
-            result = await formatter.ReadAsync(context);
-
-            // Assert
-            Assert.NotNull(result);
-            Assert.False(result.HasError);
-            model = Assert.IsType<TestLevelOne>(result.Model);
-
             Assert.Equal(expectedInt, model.SampleInt);
             Assert.Equal(expectedString, model.sampleString);
             Assert.Equal(

src/Mvc/Mvc.NewtonsoftJson/src/NewtonsoftJsonInputFormatter.cs

@@ -3,13 +3,11 @@
 
 using System;
 using System.Buffers;
-using System.Diagnostics;
 using System.IO;
 using System.Runtime.ExceptionServices;
 using System.Text;
 using System.Threading;
 using System.Threading.Tasks;
-using Microsoft.AspNetCore.Http;
 using Microsoft.AspNetCore.Mvc.ModelBinding;
 using Microsoft.AspNetCore.Mvc.NewtonsoftJson;
 using Microsoft.AspNetCore.WebUtilities;
@@ -24,6 +22,7 @@ namespace Microsoft.AspNetCore.Mvc.Formatters
     /// </summary>
     public class NewtonsoftJsonInputFormatter : TextInputFormatter, IInputFormatterExceptionPolicy
     {
+        private const int DefaultMemoryThreshold = 1024 * 30;
         private readonly IArrayPool<char> _charPool;
         private readonly ILogger _logger;
         private readonly ObjectPoolProvider _objectPoolProvider;
@@ -129,117 +128,128 @@ namespace Microsoft.AspNetCore.Mvc.Formatters
 
             var suppressInputFormatterBuffering = _options.SuppressInputFormatterBuffering;
 
+            var readStream = request.Body;
             if (!request.Body.CanSeek && !suppressInputFormatterBuffering)
             {
                 // JSON.Net does synchronous reads. In order to avoid blocking on the stream, we asynchronously
                 // read everything into a buffer, and then seek back to the beginning.
-                request.EnableBuffering();
-                Debug.Assert(request.Body.CanSeek);
+                var memoryThreshold = DefaultMemoryThreshold;
+                if (request.ContentLength.HasValue && request.ContentLength.Value > 0 && request.ContentLength.Value < memoryThreshold)
+                {
+                    // If the Content-Length is known and is smaller than the default buffer size, use it.
+                    memoryThreshold = (int)request.ContentLength.Value;
+                }
 
-                await request.Body.DrainAsync(CancellationToken.None);
-                request.Body.Seek(0L, SeekOrigin.Begin);
+                readStream = new FileBufferingReadStream(request.Body, memoryThreshold);
+
+                await readStream.DrainAsync(CancellationToken.None);
+                readStream.Seek(0L, SeekOrigin.Begin);
             }
 
-            using (var streamReader = context.ReaderFactory(request.Body, encoding))
+            var successful = true;
+            Exception exception = null;
+            object model;
+
+            using (var streamReader = context.ReaderFactory(readStream, encoding))
             {
-                using (var jsonReader = new JsonTextReader(streamReader))
+                using var jsonReader = new JsonTextReader(streamReader);
+                jsonReader.ArrayPool = _charPool;
+                jsonReader.CloseInput = false;
+
+                var type = context.ModelType;
+                var jsonSerializer = CreateJsonSerializer(context);
+                jsonSerializer.Error += ErrorHandler;
+                try
                 {
-                    jsonReader.ArrayPool = _charPool;
-                    jsonReader.CloseInput = false;
+                    model = jsonSerializer.Deserialize(jsonReader, type);
+                }
+                finally
+                {
+                    // Clean up the error handler since CreateJsonSerializer() pools instances.
+                    jsonSerializer.Error -= ErrorHandler;
+                    ReleaseJsonSerializer(jsonSerializer);
 
-                    var successful = true;
-                    Exception exception = null;
-                    void ErrorHandler(object sender, Newtonsoft.Json.Serialization.ErrorEventArgs eventArgs)
+                    if (readStream is FileBufferingReadStream fileBufferingReadStream)
                     {
-                        successful = false;
-
-                        // When ErrorContext.Path does not include ErrorContext.Member, add Member to form full path.
-                        var path = eventArgs.ErrorContext.Path;
-                        var member = eventArgs.ErrorContext.Member?.ToString();
-                        var addMember = !string.IsNullOrEmpty(member);
-                        if (addMember)
-                        {
-                            // Path.Member case (path.Length < member.Length) needs no further checks.
-                            if (path.Length == member.Length)
-                            {
-                                // Add Member in Path.Memb case but not for Path.Path.
-                                addMember = !string.Equals(path, member, StringComparison.Ordinal);
-                            }
-                            else if (path.Length > member.Length)
-                            {
-                                // Finally, check whether Path already ends with Member.
-                                if (member[0] == '[')
-                                {
-                                    addMember = !path.EndsWith(member, StringComparison.Ordinal);
-                                }
-                                else
-                                {
-                                    addMember = !path.EndsWith("." + member, StringComparison.Ordinal);
-                                }
-                            }
-                        }
-
-                        if (addMember)
-                        {
-                            path = ModelNames.CreatePropertyModelName(path, member);
-                        }
-
-                        // Handle path combinations such as ""+"Property", "Parent"+"Property", or "Parent"+"[12]".
-                        var key = ModelNames.CreatePropertyModelName(context.ModelName, path);
-
-                        exception = eventArgs.ErrorContext.Error;
-
-                        var metadata = GetPathMetadata(context.Metadata, path);
-                        var modelStateException = WrapExceptionForModelState(exception);
-                        context.ModelState.TryAddModelError(key, modelStateException, metadata);
-
-                        _logger.JsonInputException(exception);
-
-                        // Error must always be marked as handled
-                        // Failure to do so can cause the exception to be rethrown at every recursive level and
-                        // overflow the stack for x64 CLR processes
-                        eventArgs.ErrorContext.Handled = true;
+                        fileBufferingReadStream.Dispose();
                     }
+                }
+            }
 
-                    var type = context.ModelType;
-                    var jsonSerializer = CreateJsonSerializer(context);
-                    jsonSerializer.Error += ErrorHandler;
-                    object model;
-                    try
-                    {
-                        model = jsonSerializer.Deserialize(jsonReader, type);
-                    }
-                    finally
-                    {
-                        // Clean up the error handler since CreateJsonSerializer() pools instances.
-                        jsonSerializer.Error -= ErrorHandler;
-                        ReleaseJsonSerializer(jsonSerializer);
-                    }
+            if (successful)
+            {
+                if (model == null && !context.TreatEmptyInputAsDefaultValue)
+                {
+                    // Some nonempty inputs might deserialize as null, for example whitespace,
+                    // or the JSON-encoded value "null". The upstream BodyModelBinder needs to
+                    // be notified that we don't regard this as a real input so it can register
+                    // a model binding error.
+                    return InputFormatterResult.NoValue();
+                }
+                else
+                {
+                    return InputFormatterResult.Success(model);
+                }
+            }
 
-                    if (successful)
+            if (!(exception is JsonException || exception is OverflowException))
+            {
+                var exceptionDispatchInfo = ExceptionDispatchInfo.Capture(exception);
+                exceptionDispatchInfo.Throw();
+            }
+
+            return InputFormatterResult.Failure();
+
+            void ErrorHandler(object sender, Newtonsoft.Json.Serialization.ErrorEventArgs eventArgs)
+            {
+                successful = false;
+
+                // When ErrorContext.Path does not include ErrorContext.Member, add Member to form full path.
+                var path = eventArgs.ErrorContext.Path;
+                var member = eventArgs.ErrorContext.Member?.ToString();
+                var addMember = !string.IsNullOrEmpty(member);
+                if (addMember)
+                {
+                    // Path.Member case (path.Length < member.Length) needs no further checks.
+                    if (path.Length == member.Length)
                     {
-                        if (model == null && !context.TreatEmptyInputAsDefaultValue)
+                        // Add Member in Path.Memb case but not for Path.Path.
+                        addMember = !string.Equals(path, member, StringComparison.Ordinal);
+                    }
+                    else if (path.Length > member.Length)
+                    {
+                        // Finally, check whether Path already ends with Member.
+                        if (member[0] == '[')
                         {
-                            // Some nonempty inputs might deserialize as null, for example whitespace,
-                            // or the JSON-encoded value "null". The upstream BodyModelBinder needs to
-                            // be notified that we don't regard this as a real input so it can register
-                            // a model binding error.
-                            return InputFormatterResult.NoValue();
+                            addMember = !path.EndsWith(member, StringComparison.Ordinal);
                         }
                         else
                         {
-                            return InputFormatterResult.Success(model);
+                            addMember = !path.EndsWith("." + member, StringComparison.Ordinal);
                         }
                     }
-
-                    if (!(exception is JsonException || exception is OverflowException))
-                    {
-                        var exceptionDispatchInfo = ExceptionDispatchInfo.Capture(exception);
-                        exceptionDispatchInfo.Throw();
-                    }
-
-                    return InputFormatterResult.Failure();
                 }
+
+                if (addMember)
+                {
+                    path = ModelNames.CreatePropertyModelName(path, member);
+                }
+
+                // Handle path combinations such as ""+"Property", "Parent"+"Property", or "Parent"+"[12]".
+                var key = ModelNames.CreatePropertyModelName(context.ModelName, path);
+
+                exception = eventArgs.ErrorContext.Error;
+
+                var metadata = GetPathMetadata(context.Metadata, path);
+                var modelStateException = WrapExceptionForModelState(exception);
+                context.ModelState.TryAddModelError(key, modelStateException, metadata);
+
+                _logger.JsonInputException(exception);
+
+                // Error must always be marked as handled
+                // Failure to do so can cause the exception to be rethrown at every recursive level and
+                // overflow the stack for x64 CLR processes
+                eventArgs.ErrorContext.Handled = true;
             }
         }
 

src/Mvc/Mvc.NewtonsoftJson/test/NewtonsoftJsonInputFormatterTest.cs

@@ -40,7 +40,7 @@ namespace Microsoft.AspNetCore.Mvc.Formatters
             var contentBytes = Encoding.UTF8.GetBytes(content);
             var httpContext = new DefaultHttpContext();
             httpContext.Features.Set<IHttpResponseFeature>(new TestResponseFeature());
-            httpContext.Request.Body = new NonSeekableReadStream(contentBytes);
+            httpContext.Request.Body = new NonSeekableReadStream(contentBytes, allowSyncReads: false);
             httpContext.Request.ContentType = "application/json";
 
             var formatterContext = CreateInputFormatterContext(typeof(User), httpContext);
@@ -54,18 +54,6 @@ namespace Microsoft.AspNetCore.Mvc.Formatters
             var userModel = Assert.IsType<User>(result.Model);
             Assert.Equal("Person Name", userModel.Name);
             Assert.Equal(30, userModel.Age);
-
-            Assert.True(httpContext.Request.Body.CanSeek);
-            httpContext.Request.Body.Seek(0L, SeekOrigin.Begin);
-
-            result = await formatter.ReadAsync(formatterContext);
-
-            // Assert
-            Assert.False(result.HasError);
-
-            userModel = Assert.IsType<User>(result.Model);
-            Assert.Equal("Person Name", userModel.Name);
-            Assert.Equal(30, userModel.Age);
         }
 
         [Fact]
@@ -102,13 +90,6 @@ namespace Microsoft.AspNetCore.Mvc.Formatters
             var userModel = Assert.IsType<User>(result.Model);
             Assert.Equal("Person Name", userModel.Name);
             Assert.Equal(30, userModel.Age);
-
-            Assert.False(httpContext.Request.Body.CanSeek);
-            result = await formatter.ReadAsync(formatterContext);
-
-            // Assert
-            Assert.False(result.HasError);
-            Assert.Null(result.Model);
         }
 
         [Fact]

src/Mvc/Mvc.NewtonsoftJson/test/NewtonsoftJsonPatchInputFormatterTest.cs

@@ -41,7 +41,7 @@ namespace Microsoft.AspNetCore.Mvc.Formatters
 
             var httpContext = new DefaultHttpContext();
             httpContext.Features.Set<IHttpResponseFeature>(new TestResponseFeature());
-            httpContext.Request.Body = new NonSeekableReadStream(contentBytes);
+            httpContext.Request.Body = new NonSeekableReadStream(contentBytes, allowSyncReads: false);
             httpContext.Request.ContentType = "application/json";
 
             var formatterContext = CreateInputFormatterContext(typeof(JsonPatchDocument<Customer>), httpContext);
@@ -55,18 +55,6 @@ namespace Microsoft.AspNetCore.Mvc.Formatters
             Assert.Equal("add", patchDocument.Operations[0].op);
             Assert.Equal("Customer/Name", patchDocument.Operations[0].path);
             Assert.Equal("John", patchDocument.Operations[0].value);
-
-            Assert.True(httpContext.Request.Body.CanSeek);
-            httpContext.Request.Body.Seek(0L, SeekOrigin.Begin);
-
-            result = await formatter.ReadAsync(formatterContext);
-
-            // Assert
-            Assert.False(result.HasError);
-            patchDocument = Assert.IsType<JsonPatchDocument<Customer>>(result.Model);
-            Assert.Equal("add", patchDocument.Operations[0].op);
-            Assert.Equal("Customer/Name", patchDocument.Operations[0].path);
-            Assert.Equal("John", patchDocument.Operations[0].value);
         }
 
         [Fact]

src/Mvc/shared/Mvc.Core.TestCommon/NonSeekableReadableStream.cs

@@ -11,15 +11,17 @@ namespace Microsoft.AspNetCore.Mvc
     public class NonSeekableReadStream : Stream
     {
         private Stream _inner;
+        private readonly bool _allowSyncReads;
 
-        public NonSeekableReadStream(byte[] data)
-            : this(new MemoryStream(data))
+        public NonSeekableReadStream(byte[] data, bool allowSyncReads = true)
+            : this(new MemoryStream(data), allowSyncReads)
         {
         }
 
-        public NonSeekableReadStream(Stream inner)
+        public NonSeekableReadStream(Stream inner, bool allowSyncReads)
         {
             _inner = inner;
+            _allowSyncReads = allowSyncReads;
         }
 
         public override bool CanRead => _inner.CanRead;
@@ -61,6 +63,11 @@ namespace Microsoft.AspNetCore.Mvc
 
         public override int Read(byte[] buffer, int offset, int count)
         {
+            if (!_allowSyncReads)
+            {
+                throw new InvalidOperationException("Cannot perform synchronous reads");
+            }
+
             count = Math.Max(count, 1);
             return _inner.Read(buffer, offset, count);
         }