Prevent synchronous writes when using Razor #9395
Conversation
pranavkm
force-pushed
the
prkrishn/view-buffering
branch
2 times, most recently
from
80b1ee1
to
6b2f24b
Compare
* 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
pranavkm
force-pushed
the
prkrishn/view-buffering
branch
from
6b2f24b
to
833b5db
Compare
| /// <inheritdoc /> | ||
| public override void Write(char value) | ||
| { | ||
| if (IsBuffering) |
There was a problem hiding this comment.
Some background - ViewBufferTextWriter had a dual mode of operation - after a Flush, all writes directly went to the underlying TextWriter (i.e. HttpResponseStreamWriter). With large enough content, we'll run out of buffer in the writer and result in a synchronous write that throws.
There isn't a very good reason for us to stop buffering once a flush is performed, it's really not an important \ well-defined requirement of FlushAsync .
There was a problem hiding this comment.
Yeah I agree. This seems like such an obvious improvment, I'm sad we didn't think of it befores.
| @@ -62,6 +77,7 @@ public class ViewComponentResultExecutor : IActionResultExecutor<ViewComponentRe | |||
| _htmlEncoder = htmlEncoder; | |||
| _modelMetadataProvider = modelMetadataProvider; | |||
| _tempDataDictionaryFactory = tempDataDictionaryFactory; | |||
| _writerFactory = writerFactory; | |||
There was a problem hiding this comment.
Explicitly allowing null. We will get the service in the body of ExecuteAsync
| var viewComponentResult = await GetViewComponentResult(viewComponentHelper, _logger, result); | ||
| viewComponentResult.WriteTo(writer, _htmlEncoder); | ||
|
|
||
| if (viewComponentResult is ViewBuffer viewBuffer) |
There was a problem hiding this comment.
https://github.com/aspnet/AspNetCore/blob/master/src/Mvc/Mvc.ViewFeatures/src/ViewComponents/DefaultViewComponentHelper.cs#L176 returns ViewBuffer. It's a bit unfortunate we have to downcast this since a user could have customized the implementation.
There was a problem hiding this comment.
Yeah I dunno why we made this stuff public. Sadbeans
| } | ||
| else | ||
| { | ||
| using var memoryStream = new MemoryStream(); |
There was a problem hiding this comment.
I'll change this to use the FileBufferingWriteStream once that's in.
There was a problem hiding this comment.
Worth a comment with the issue # to rememberize it?
|
/* No comments for this PR. */ Any updates for #9015? |
I split this one off as a separate PR just because I realized it had mostly unrelated changes. I'll get the other one updated soon |
|
This comment was made automatically. If there is a problem contact ryanbrandenburg. I've triaged the above build. I've created/commented on the following issue(s) |
Eilon
added
the
area-mvc
| { | ||
| syncIOFeature.AllowSynchronousIO = true; | ||
| } | ||
| _writerFactory ??= context.HttpContext.RequestServices.GetRequiredService<IHttpResponseStreamWriterFactory>(); |
Related to #6397
Fixes #4885