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

Prevent synchronous writes when using Razor #9395

Merged
merged 2 commits into from
Apr 16, 2019
Merged

Prevent synchronous writes when using Razor #9395

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
833b5db
Prevent synchronous writes when using Razor
pranavkm Apr 15, 2019
4ea68b9
Always refs
pranavkm Apr 16, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 5 additions & 6 deletions src/Mvc/Mvc.Razor/src/RazorPageBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -370,12 +370,7 @@ public virtual void Write(object value)
var encoder = HtmlEncoder;
if (value is IHtmlContent htmlContent)
{
var bufferedWriter = writer as ViewBufferTextWriter;
if (bufferedWriter == null || !bufferedWriter.IsBuffering)
{
htmlContent.WriteTo(writer, encoder);
}
else
if (writer is ViewBufferTextWriter bufferedWriter)
{
if (value is IHtmlContentContainer htmlContentContainer)
{
Expand All @@ -389,6 +384,10 @@ public virtual void Write(object value)
bufferedWriter.Buffer.AppendHtml(htmlContent);
}
}
else
{
htmlContent.WriteTo(writer, encoder);
}

return;
}
Expand Down
33 changes: 15 additions & 18 deletions src/Mvc/Mvc.Razor/src/RazorView.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -233,7 +233,7 @@ private async Task RenderLayoutAsync(
// (including the layout page we just rendered).
while (!string.IsNullOrEmpty(previousPage.Layout))
{
if (!bodyWriter.IsBuffering)
if (bodyWriter.Flushed)
{
// Once a call to RazorPage.FlushAsync is made, we can no longer render Layout pages - content has
// already been written to the client and the layout content would be appended rather than surround
Expand Down Expand Up @@ -274,25 +274,22 @@ private async Task RenderLayoutAsync(
layoutPage.EnsureRenderedBodyOrSections();
}

if (bodyWriter.IsBuffering)
// We've got a bunch of content in the view buffer. How to best deal with it
// really depends on whether or not we're writing directly to the output or if we're writing to
// another buffer.
if (context.Writer is ViewBufferTextWriter viewBufferTextWriter)
{
// If IsBuffering - then we've got a bunch of content in the view buffer. How to best deal with it
// really depends on whether or not we're writing directly to the output or if we're writing to
// another buffer.
var viewBufferTextWriter = context.Writer as ViewBufferTextWriter;
if (viewBufferTextWriter == null || !viewBufferTextWriter.IsBuffering)
{
// This means we're writing to a 'real' writer, probably to the actual output stream.
// We're using PagedBufferedTextWriter here to 'smooth' synchronous writes of IHtmlContent values.
using (var writer = _bufferScope.CreateWriter(context.Writer))
{
await bodyWriter.Buffer.WriteToAsync(writer, _htmlEncoder);
}
}
else
// This means we're writing to another buffer. Use MoveTo to combine them.
bodyWriter.Buffer.MoveTo(viewBufferTextWriter.Buffer);
}
else
{
// This means we're writing to a 'real' writer, probably to the actual output stream.
// We're using PagedBufferedTextWriter here to 'smooth' synchronous writes of IHtmlContent values.
using (var writer = _bufferScope.CreateWriter(context.Writer))
{
// This means we're writing to another buffer. Use MoveTo to combine them.
bodyWriter.Buffer.MoveTo(viewBufferTextWriter.Buffer);
await bodyWriter.Buffer.WriteToAsync(writer, _htmlEncoder);
await writer.FlushAsync();
}
}
}
Expand Down
181 changes: 39 additions & 142 deletions src/Mvc/Mvc.ViewFeatures/src/Buffers/ViewBufferTextWriter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,25 +86,20 @@ public ViewBufferTextWriter(ViewBuffer buffer, Encoding encoding, HtmlEncoder ht
/// <inheritdoc />
public override Encoding Encoding { get; }

/// <inheritdoc />
public bool IsBuffering { get; private set; } = true;

/// <summary>
/// Gets the <see cref="ViewBuffer"/>.
/// </summary>
public ViewBuffer Buffer { get; }

/// <summary>
/// Gets a value that indiciates if <see cref="Flush"/> or <see cref="FlushAsync" /> was invoked.
/// </summary>
public bool Flushed { get; private set; }

/// <inheritdoc />
public override void Write(char value)
{
if (IsBuffering)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 .

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I agree. This seems like such an obvious improvment, I'm sad we didn't think of it befores.

{
Buffer.AppendHtml(value.ToString());
}
else
{
_inner.Write(value);
}
Buffer.AppendHtml(value.ToString());
}

/// <inheritdoc />
Expand All @@ -125,14 +120,7 @@ public override void Write(char[] buffer, int index, int count)
throw new ArgumentOutOfRangeException(nameof(count));
}

if (IsBuffering)
{
Buffer.AppendHtml(new string(buffer, index, count));
}
else
{
_inner.Write(buffer, index, count);
}
Buffer.AppendHtml(new string(buffer, index, count));
}

/// <inheritdoc />
Expand All @@ -143,14 +131,7 @@ public override void Write(string value)
return;
}

if (IsBuffering)
{
Buffer.AppendHtml(value);
}
else
{
_inner.Write(value);
}
Buffer.AppendHtml(value);
}

/// <inheritdoc />
Expand Down Expand Up @@ -186,14 +167,7 @@ public void Write(IHtmlContent value)
return;
}

if (IsBuffering)
{
Buffer.AppendHtml(value);
}
else
{
value.WriteTo(_inner, _htmlEncoder);
}
Buffer.AppendHtml(value);
}

/// <summary>
Expand All @@ -207,14 +181,7 @@ public void Write(IHtmlContentContainer value)
return;
}

if (IsBuffering)
{
value.MoveTo(Buffer);
}
else
{
value.WriteTo(_inner, _htmlEncoder);
}
value.MoveTo(Buffer);
}

/// <inheritdoc />
Expand Down Expand Up @@ -245,15 +212,8 @@ public override void WriteLine(object value)
/// <inheritdoc />
public override Task WriteAsync(char value)
{
if (IsBuffering)
{
Buffer.AppendHtml(value.ToString());
return Task.CompletedTask;
}
else
{
return _inner.WriteAsync(value);
}
Buffer.AppendHtml(value.ToString());
return Task.CompletedTask;
}

/// <inheritdoc />
Expand All @@ -273,121 +233,64 @@ public override Task WriteAsync(char[] buffer, int index, int count)
throw new ArgumentOutOfRangeException(nameof(count));
}

if (IsBuffering)
{
Buffer.AppendHtml(new string(buffer, index, count));
return Task.CompletedTask;
}
else
{
return _inner.WriteAsync(buffer, index, count);
}
Buffer.AppendHtml(new string(buffer, index, count));
return Task.CompletedTask;
}

/// <inheritdoc />
public override Task WriteAsync(string value)
{
if (IsBuffering)
{
Buffer.AppendHtml(value);
return Task.CompletedTask;
}
else
{
return _inner.WriteAsync(value);
}
Buffer.AppendHtml(value);
return Task.CompletedTask;
}

/// <inheritdoc />
public override void WriteLine()
{
if (IsBuffering)
{
Buffer.AppendHtml(NewLine);
}
else
{
_inner.WriteLine();
}
Buffer.AppendHtml(NewLine);
}

/// <inheritdoc />
public override void WriteLine(string value)
{
if (IsBuffering)
{
Buffer.AppendHtml(value);
Buffer.AppendHtml(NewLine);
}
else
{
_inner.WriteLine(value);
}
Buffer.AppendHtml(value);
Buffer.AppendHtml(NewLine);
}

/// <inheritdoc />
public override Task WriteLineAsync(char value)
{
if (IsBuffering)
{
Buffer.AppendHtml(value.ToString());
Buffer.AppendHtml(NewLine);
return Task.CompletedTask;
}
else
{
return _inner.WriteLineAsync(value);
}
Buffer.AppendHtml(value.ToString());
Buffer.AppendHtml(NewLine);
return Task.CompletedTask;
}

/// <inheritdoc />
public override Task WriteLineAsync(char[] value, int start, int offset)
{
if (IsBuffering)
{
Buffer.AppendHtml(new string(value, start, offset));
Buffer.AppendHtml(NewLine);
return Task.CompletedTask;
}
else
{
return _inner.WriteLineAsync(value, start, offset);
}
Buffer.AppendHtml(new string(value, start, offset));
Buffer.AppendHtml(NewLine);
return Task.CompletedTask;

}

/// <inheritdoc />
public override Task WriteLineAsync(string value)
{
if (IsBuffering)
{
Buffer.AppendHtml(value);
Buffer.AppendHtml(NewLine);
return Task.CompletedTask;
}
else
{
return _inner.WriteLineAsync(value);
}
Buffer.AppendHtml(value);
Buffer.AppendHtml(NewLine);
return Task.CompletedTask;
}

/// <inheritdoc />
public override Task WriteLineAsync()
{
if (IsBuffering)
{
Buffer.AppendHtml(NewLine);
return Task.CompletedTask;
}
else
{
return _inner.WriteLineAsync();
}
Buffer.AppendHtml(NewLine);
return Task.CompletedTask;
}

/// <summary>
/// Copies the buffered content to the unbuffered writer and invokes flush on it.
/// Additionally causes this instance to no longer buffer and direct all write operations
/// to the unbuffered writer.
/// </summary>
public override void Flush()
{
Expand All @@ -396,20 +299,16 @@ public override void Flush()
return;
}

if (IsBuffering)
{
IsBuffering = false;
Buffer.WriteTo(_inner, _htmlEncoder);
Buffer.Clear();
}
Flushed = true;

Buffer.WriteTo(_inner, _htmlEncoder);
Buffer.Clear();

_inner.Flush();
}

/// <summary>
/// Copies the buffered content to the unbuffered writer and invokes flush on it.
/// Additionally causes this instance to no longer buffer and direct all write operations
/// to the unbuffered writer.
/// </summary>
/// <returns>A <see cref="Task"/> that represents the asynchronous copy and flush operations.</returns>
public override async Task FlushAsync()
Expand All @@ -419,12 +318,10 @@ public override async Task FlushAsync()
return;
}

if (IsBuffering)
{
IsBuffering = false;
await Buffer.WriteToAsync(_inner, _htmlEncoder);
Buffer.Clear();
}
Flushed = true;

await Buffer.WriteToAsync(_inner, _htmlEncoder);
Buffer.Clear();

await _inner.FlushAsync();
}
Expand Down
Loading