Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fully buffer XML, JSON, and Razor request and ViewComponentResultExecutor response bodies #6397

Closed
Tratcher opened this issue Jan 4, 2019 · 8 comments · Fixed by #9015
Closed

Fully buffer XML, JSON, and Razor request and ViewComponentResultExecutor response bodies #6397

Tratcher opened this issue Jan 4, 2019 · 8 comments · Fixed by #9015
Assignees
@pranavkm
Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates Done This issue has been fixed enhancement This issue represents an ask for new feature or an enhancement to an existing one
Milestone
3.0.0-preview5

Comments

@Tratcher
Copy link
Member

Tratcher commented Jan 4, 2019

Is your feature request related to a problem? Please describe.

Sync IO is a frequent source of thread pool starvation. As such #5120 bans sync IO operations for each server by default. Unfortunately the MVC XML and JSON serializers and deserializers rely on sync IO, as does the Razor view engine.

Describe the solution you'd like

Fully buffer the request and response bodies for the XML, Newtonsoft.JSON, and Razor scenarios. The new JSON APIs for 3.0 should provide async implementations. The current implementations already rely on partial buffering, but perform sync IO if those buffers fill/drain.

This has a frequently requested side benefit of allowing the app to recover from response serialization errors and return a meaningful error message.

Here are a few example test failures:

Microsoft.AspNetCore.Mvc.FunctionalTests.JsonOutputFormatterTests.SerializableErrorIsReturnedInExpectedFormat...
System.InvalidOperationException : Synchronous operations are disallowed. Call ReadAsync or set AllowSynchronousIO to true.
 at Microsoft.AspNetCore.TestHost.AsyncStreamWrapper.Read(Byte[] buffer, Int32 offset, Int32 count) in /_/src/Hosting/TestHost/src/AsyncStreamWrapper.cs:line 50
   at System.IO.Stream.ReadByte()
   at Microsoft.AspNetCore.Mvc.Infrastructure.NonDisposableStream.ReadByte() in /_/src/Mvc/src/Microsoft.AspNetCore.Mvc.Core/Infrastructure/NonDisposableStream.cs:line 134
   at System.Xml.EncodingStreamWrapper.ReadBOMEncoding(Boolean notOutOfBand)
   at System.Xml.EncodingStreamWrapper..ctor(Stream stream, Encoding encoding)
   at System.Xml.XmlUTF8TextReader.SetInput(Stream stream, Encoding encoding, XmlDictionaryReaderQuotas quotas, OnXmlDictionaryReaderClose onClose)
   at System.Xml.XmlDictionaryReader.CreateTextReader(Stream stream, Encoding encoding, XmlDictionaryReaderQuotas quotas, OnXmlDictionaryReaderClose onClose)
   at Microsoft.AspNetCore.Mvc.Formatters.XmlDataContractSerializerInputFormatter.CreateXmlReader(Stream readStream, Encoding encoding) in /_/src/Mvc/src/Microsoft.AspNetCore.Mvc.Formatters.Xml/XmlDataContractSerializerInputFormatter.cs:line 213
   at Microsoft.AspNetCore.Mvc.Formatters.XmlDataContractSerializerInputFormatter.ReadRequestBodyAsync(InputFormatterContext context, Encoding encoding) in /_/src/Mvc/src/Microsoft.AspNetCore.Mvc.Formatters.Xml/XmlDataContractSerializerInputFormatter.cs:line 159
   at Microsoft.AspNetCore.Mvc.ModelBinding.Binders.BodyModelBinder.BindModelAsync(ModelBindingContext bindingContext) in /_/src/Mvc/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/BodyModelBinder.cs:line 157
   at Microsoft.AspNetCore.Mvc.ModelBinding.ParameterBinder.BindModelAsync(ActionContext actionContext, IModelBinder modelBinder, IValueProvider valueProvider, ParameterDescriptor parameter, ModelMetadata metadata, Object value) in /_/src/Mvc/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/ParameterBinder.cs:line 258
   at Microsoft.AspNetCore.Mvc.Controllers.ControllerBinderDelegateProvider.<>c__DisplayClass0_0.<<CreateBinderDelegate>g__Bind|0>d.MoveNext() in /_/src/Mvc/src/Microsoft.AspNetCore.Mvc.Core/Controllers/ControllerBinderDelegateProvider.cs:line 76

Microsoft.AspNetCore.Mvc.FunctionalTests.XmlSerializerFormattersWrappingTest.ValidationProblem...
System.InvalidOperationException : Synchronous operations are disallowed. Call WriteAsync or set AllowSynchronousIO to true.
   at Microsoft.AspNetCore.TestHost.ResponseStream.Write(Byte[] buffer, Int32 offset, Int32 count) in /_/src/Hosting/TestHost/src/ResponseStream.cs:line 155
   at Microsoft.AspNetCore.WebUtilities.HttpResponseStreamWriter.FlushInternal(Boolean flushEncoder) in /_/src/Http/WebUtilities/src/HttpResponseStreamWriter.cs:line 336
   at Microsoft.AspNetCore.WebUtilities.HttpResponseStreamWriter.Flush() in /_/src/Http/WebUtilities/src/HttpResponseStreamWriter.cs:line 282
   at System.Xml.XmlEncodedRawTextWriter.Flush()
   at System.Xml.XmlWellFormedWriter.Flush()
   at System.Xml.Serialization.XmlSerializer.Serialize(XmlWriter xmlWriter, Object o, XmlSerializerNamespaces namespaces, String encodingStyle, String id)
   at System.Xml.Serialization.XmlSerializer.Serialize(XmlWriter xmlWriter, Object o)
   at Microsoft.AspNetCore.Mvc.Formatters.XmlSerializerOutputFormatter.Serialize(XmlSerializer xmlSerializer, XmlWriter xmlWriter, Object value) in /_/src/Mvc/src/Microsoft.AspNetCore.Mvc.Formatters.Xml/XmlSerializerOutputFormatter.cs:line 257
   at Microsoft.AspNetCore.Mvc.Formatters.XmlSerializerOutputFormatter.WriteResponseBodyAsync(OutputFormatterWriteContext context, Encoding selectedEncoding) in /_/src/Mvc/src/Microsoft.AspNetCore.Mvc.Formatters.Xml/XmlSerializerOutputFormatter.cs:line 237
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.InvokeResultAsync(IActionResult result) in /_/src/Mvc/src/Microsoft.AspNetCore.Mvc.Core/Infrastructure/ResourceInvoker.cs:line 137
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.InvokeNextResultFilterAsync[TFilter,TFilterAsync]() in /_/src/Mvc/src/Microsoft.AspNetCore.Mvc.Core/Infrastructure/ResourceInvoker.cs:line 1089
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.Rethrow(ResultExecutedContext context) in /_/src/Mvc/src/Microsoft.AspNetCore.Mvc.Core/Infrastructure/ResourceInvoker.cs:line 1192
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.ResultNext[TFilter,TFilterAsync](State& next, Scope& scope, Object& state, Boolean& isCompleted) in /_/src/Mvc/src/Microsoft.AspNetCore.Mvc.Core/Infrastructure/ResourceInvoker.cs:line 1071
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.InvokeResultFilters() in /_/src/Mvc/src/Microsoft.AspNetCore.Mvc.Core/Infrastructure/ResourceInvoker.cs:line 850
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.InvokeNextResourceFilter() in /_/src/Mvc/src/Microsoft.AspNetCore.Mvc.Core/Infrastructure/ResourceInvoker.cs:line 790
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.Rethrow(ResourceExecutedContext context) in /_/src/Mvc/src/Microsoft.AspNetCore.Mvc.Core/Infrastructure/ResourceInvoker.cs:line 1146
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.Next(State& next, Scope& scope, Object& state, Boolean& isCompleted) in /_/src/Mvc/src/Microsoft.AspNetCore.Mvc.Core/Infrastructure/ResourceInvoker.cs:line 752
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.InvokeFilterPipelineAsync() in /_/src/Mvc/src/Microsoft.AspNetCore.Mvc.Core/Infrastructure/ResourceInvoker.cs:line 122
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.InvokeAsync() in /_/src/Mvc/src/Microsoft.AspNetCore.Mvc.Core/Infrastructure/ResourceInvoker.cs:line 80

Microsoft.AspNetCore.Mvc.FunctionalTests.InputFormatterTests.ValidationUsesModelMetadataFrom...
System.InvalidOperationException : Synchronous operations are disallowed. Call ReadAsync or set AllowSynchronousIO to true.
  at Microsoft.AspNetCore.TestHost.AsyncStreamWrapper.Read(Byte[] buffer, Int32 offset, Int32 count) in /_/src/Hosting/TestHost/src/AsyncStreamWrapper.cs:line 50
   at Microsoft.AspNetCore.WebUtilities.HttpRequestStreamReader.ReadIntoBuffer() in /_/src/Http/WebUtilities/src/HttpRequestStreamReader.cs:line 321
   at Microsoft.AspNetCore.WebUtilities.HttpRequestStreamReader.Read(Char[] buffer, Int32 index, Int32 count) in /_/src/Http/WebUtilities/src/HttpRequestStreamReader.cs:line 168
   at Newtonsoft.Json.JsonTextReader.ReadData(Boolean append, Int32 charsRequired) in /_/Src/Newtonsoft.Json/JsonTextReader.cs:line 343
   at Newtonsoft.Json.JsonTextReader.ReadData(Boolean append) in /_/Src/Newtonsoft.Json/JsonTextReader.cs:line 272
   at Newtonsoft.Json.JsonTextReader.ParseValue() in /_/Src/Newtonsoft.Json/JsonTextReader.cs:line 1665
   at Newtonsoft.Json.JsonTextReader.Read() in /_/Src/Newtonsoft.Json/JsonTextReader.cs:line 419
   at Newtonsoft.Json.JsonReader.ReadForType(JsonContract contract, Boolean hasConverter) in /_/Src/Newtonsoft.Json/JsonReader.cs:line 1233
   at Newtonsoft.Json.Serialization.JsonSerializerInternalReader.Deserialize(JsonReader reader, Type objectType, Boolean checkAdditionalContent) in /_/Src/Newtonsoft.Json/Serialization/JsonSerializerInternalReader.cs:line 149
--- End of stack trace from previous location where exception was thrown ---
   at Microsoft.AspNetCore.Mvc.Formatters.JsonInputFormatter.ReadRequestBodyAsync(InputFormatterContext context, Encoding encoding) in /_/src/Mvc/src/Microsoft.AspNetCore.Mvc.Formatters.Json/JsonInputFormatter.cs:line 349
   at Microsoft.AspNetCore.Mvc.ModelBinding.Binders.BodyModelBinder.BindModelAsync(ModelBindingContext bindingContext) in /_/src/Mvc/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/BodyModelBinder.cs:line 157
   at Microsoft.AspNetCore.Mvc.ModelBinding.ParameterBinder.BindModelAsync(ActionContext actionContext, IModelBinder modelBinder, IValueProvider valueProvider, ParameterDescriptor parameter, ModelMetadata metadata, Object value) in /_/src/Mvc/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/ParameterBinder.cs:line 258
   at Microsoft.AspNetCore.Mvc.Controllers.ControllerBinderDelegateProvider.<>c__DisplayClass0_0.<<CreateBinderDelegate>g__Bind|0>d.MoveNext() in /_/src/Mvc/src/Microsoft.AspNetCore.Mvc.Core/Controllers/ControllerBinderDelegateProvider.cs:line 76
--- End of stack trace from previous location where exception was thrown ---
   at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.InvokeInnerFilterAsync() in /_/src/Mvc/src/Microsoft.AspNetCore.Mvc.Core/Infrastructure/ControllerActionInvoker.cs:line 382
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.InvokeNextResourceFilter() in /_/src/Mvc/src/Microsoft.AspNetCore.Mvc.Core/Infrastructure/ResourceInvoker.cs:line 790
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.Rethrow(ResourceExecutedContext context) in /_/src/Mvc/src/Microsoft.AspNetCore.Mvc.Core/Infrastructure/ResourceInvoker.cs:line 1146
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.Next(State& next, Scope& scope, Object& state, Boolean& isCompleted) in /_/src/Mvc/src/Microsoft.AspNetCore.Mvc.Core/Infrastructure/ResourceInvoker.cs:line 752
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.InvokeFilterPipelineAsync() in /_/src/Mvc/src/Microsoft.AspNetCore.Mvc.Core/Infrastructure/ResourceInvoker.cs:line 122

Microsoft.AspNetCore.Mvc.FunctionalTests.ViewComponentFromServicesTest.ViewComponents...
System.InvalidOperationException : Synchronous operations are disallowed. Call WriteAsync or set AllowSynchronousIO to true.
  at Microsoft.AspNetCore.TestHost.ResponseStream.Write(Byte[] buffer, Int32 offset, Int32 count) in /_/src/Hosting/TestHost/src/ResponseStream.cs:line 155
   at Microsoft.AspNetCore.WebUtilities.HttpResponseStreamWriter.FlushInternal(Boolean flushEncoder) in /_/src/Http/WebUtilities/src/HttpResponseStreamWriter.cs:line 336
   at Microsoft.AspNetCore.WebUtilities.HttpResponseStreamWriter.Dispose(Boolean disposing) in /_/src/Http/WebUtilities/src/HttpResponseStreamWriter.cs:line 301
   at System.IO.TextWriter.Dispose()
   at Microsoft.AspNetCore.Mvc.ViewFeatures.ViewComponentResultExecutor.ExecuteAsync(ActionContext context, ViewComponentResult result) in /_/src/Mvc/src/Microsoft.AspNetCore.Mvc.ViewFeatures/ViewComponentResultExecutor.cs:line 126
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.InvokeResultAsync(IActionResult result) in /_/src/Mvc/src/Microsoft.AspNetCore.Mvc.Core/Infrastructure/ResourceInvoker.cs:line 137
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.InvokeNextResultFilterAsync[TFilter,TFilterAsync]() in /_/src/Mvc/src/Microsoft.AspNetCore.Mvc.Core/Infrastructure/ResourceInvoker.cs:line 1089
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.Rethrow(ResultExecutedContext context) in /_/src/Mvc/src/Microsoft.AspNetCore.Mvc.Core/Infrastructure/ResourceInvoker.cs:line 1192
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.ResultNext[TFilter,TFilterAsync](State& next, Scope& scope, Object& state, Boolean& isCompleted) in /_/src/Mvc/src/Microsoft.AspNetCore.Mvc.Core/Infrastructure/ResourceInvoker.cs:line 1071
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.InvokeResultFilters() in /_/src/Mvc/src/Microsoft.AspNetCore.Mvc.Core/Infrastructure/ResourceInvoker.cs:line 850
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.InvokeNextResourceFilter() in /_/src/Mvc/src/Microsoft.AspNetCore.Mvc.Core/Infrastructure/ResourceInvoker.cs:line 790
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.Rethrow(ResourceExecutedContext context) in /_/src/Mvc/src/Microsoft.AspNetCore.Mvc.Core/Infrastructure/ResourceInvoker.cs:line 1146
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.Next(State& next, Scope& scope, Object& state, Boolean& isCompleted) in /_/src/Mvc/src/Microsoft.AspNetCore.Mvc.Core/Infrastructure/ResourceInvoker.cs:line 752
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.InvokeFilterPipelineAsync() in /_/src/Mvc/src/Microsoft.AspNetCore.Mvc.Core/Infrastructure/ResourceInvoker.cs:line 122
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.InvokeAsync() in /_/src/Mvc/src/Microsoft.AspNetCore.Mvc.Core/Infrastructure/ResourceInvoker.cs:line 80
   at Microsoft.AspNetCore.Routing.EndpointMiddleware.Invoke(HttpContext httpContext) in /_/src/Http/Routing/src/EndpointMiddleware.cs:line 42
   at Microsoft.AspNetCore.Routing.EndpointRoutingMiddleware.Invoke(HttpContext httpContext) in /_/src/Http/Routing/src/EndpointRoutingMiddleware.cs:line 78
   at Microsoft.AspNetCore.Mvc.FunctionalTests.CultureReplacerMiddleware.Invoke(HttpContext context) in /_/src/Mvc/test/Microsoft.AspNetCore.Mvc.FunctionalTests/Infrastructure/CultureReplacerMiddleware.cs:line 45
   at Microsoft.AspNetCore.TestHost.HttpContextBuilder.<>c__DisplayClass14_0.<<SendAsync>b__0>d.MoveNext() in /_/src/Hosting/TestHost/src/HttpContextBuilder.cs:line 74

@davidfowl @mkArtakMSFT @pranavkm
@Tratcher Tratcher added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Jan 4, 2019
@Tratcher Tratcher mentioned this issue Jan 4, 2019
Merged
@mkArtakMSFT
Copy link
Member

@rynowak, thoughts?
We've been avoiding buffering the responses and requests and that was the direction we had taken in communication with customers in the past. Should this change?

@davidfowl
Copy link
Member

Lets discuss this with @DamianEdwards and @Tratcher. We discussed making this configurable so that it would blow up if it went above some threshold by default and it could be configurable per request otherwise.

@Tratcher
Copy link
Member Author

Tratcher commented Jan 7, 2019

We may have enough control over razor to make it do smarter async flushing rather than full buffering. E.g. FlushAsync between tags if the buffer is more than 75% full.

@Tratcher
Copy link
Member Author

Offline design notes from @danroth27 and @davidfowl:
Customers don't want a response buffering size limit, it's just going to fail, get raised to MaxInt, and then cause OutOfMemory exceptions.
Proposal: Use the same file backed buffering mechanic as we do for requests, buffer the full response, and then flush once the serializer is done. HttpAbstractions can provide the buffer stream implementation, but MVC needs to flush it since it's the one that knows when the serilaizer is done.

@muratg muratg mentioned this issue Jan 11, 2019
Closed
@halter73
Copy link
Member

I hope this doesn't get used as an excuse to continue allowing Razor to do sync writes/flushes. Buffering is a decent mitigation, but not nearly as good as top-to-bottom async I/O.

@rynowak
Copy link
Member

rynowak commented Jan 12, 2019

@halter73 do you have a (non-glib) proposal for what that looks like?

For context, we already do something internally that might map pretty well to pipelines. If it is a good fit, and all 3.0 users can run on pipelines then I'd like to see what it would take to migrate Razor.

@Tratcher
Copy link
Member Author

@rynowak pipelines are available in a few places now for you to start prototyping with.

@pranavkm pranavkm mentioned this issue Feb 15, 2019
Closed
@Tratcher
Copy link
Member Author

The sync IO change has been merged with a few local opt-outs for things like the XML serilaizer. These should still be addressed.

@pranavkm pranavkm changed the title Fully buffer XML, JSON, and Razor request and response bodies Fully buffer XML, JSON, and Razor request and ViewComponentResultExecutor response bodies Mar 27, 2019
@pranavkm pranavkm assigned pranavkm and unassigned rynowak Mar 27, 2019
@pranavkm pranavkm added this to the 3.0.0-preview4 milestone Mar 27, 2019
@pranavkm pranavkm added 1 - Ready enhancement This issue represents an ask for new feature or an enhancement to an existing one and removed 1 - Ready labels Mar 27, 2019
pranavkm added a commit that referenced this issue Apr 3, 2019
@pranavkm
Buffer writes from sources of synchronous writes
2036f06 
Fixes #6397
@pranavkm pranavkm mentioned this issue Apr 3, 2019
Merged
@pranavkm pranavkm mentioned this issue Apr 15, 2019
Merged
pranavkm added a commit that referenced this issue Apr 16, 2019
@pranavkm
Buffer writes from sources of synchronous writes
692569d 
Fixes #6397
pranavkm added a commit that referenced this issue Apr 16, 2019
@pranavkm
Prevent synchronous writes when using Razor (#9395)
2be8052 
* Do not perform synchronous writes to the Response TextWriter after a Razor FlushAsync
* Use ViewBuffer to perform async writes to the response when using ViewComponentResult

Related to #6397 

Fixes #4885
@pranavkm pranavkm added Done This issue has been fixed and removed 2 - Working labels Apr 18, 2019
pranavkm added a commit that referenced this issue Apr 18, 2019
@pranavkm
Buffer writes from sources of synchronous writes (#9015)
92cae6f 
* Buffer writes from sources of synchronous writes

Fixes #6397
@Tratcher Tratcher mentioned this issue May 28, 2019
Closed
@ghost ghost locked as resolved and limited conversation to collaborators Dec 3, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@pranavkm pranavkm

Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates Done This issue has been fixed enhancement This issue represents an ask for new feature or an enhancement to an existing one
Projects
None yet
Milestone
3.0.0-preview5
Development

Successfully merging a pull request may close this issue.

Buffer writes from sources of synchronous writes dotnet/aspnetcore
6 participants
@halter73 @davidfowl @pranavkm @rynowak @Tratcher @mkArtakMSFT