Skip to content

Make StartAsync not throw if we haven't started the response #8199

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

Merged
merged 16 commits into from
Mar 8, 2019
Merged

Make StartAsync not throw if we haven't started the response #8199

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
13b969a
temp
jkotalik Feb 28, 2019
5b94f74
WIP
jkotalik Mar 1, 2019
3d6d163
Working
jkotalik Mar 4, 2019
34e3381
Go with the simple approach
jkotalik Mar 5, 2019
61640ae
Comments
jkotalik Mar 5, 2019
009380f
Feedback
jkotalik Mar 5, 2019
33d8cd6
Fix test
jkotalik Mar 5, 2019
e63dac4
Feedback
jkotalik Mar 5, 2019
08f94ac
bit more feedback
jkotalik Mar 5, 2019
0ec739d
Remove min buffer size
jkotalik Mar 6, 2019
cbf8d68
whoops
jkotalik Mar 6, 2019
da8b6ed
update
jkotalik Mar 6, 2019
028e19c
Reset state
jkotalik Mar 6, 2019
039fdd3
Feedback, adding more tests, and making the grass a little greener
jkotalik Mar 7, 2019
959a31a
Cleanup some comments
jkotalik Mar 7, 2019
63162c2
Update refs
jkotalik Mar 7, 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
187 changes: 178 additions & 9 deletions src/Servers/Kestrel/Core/src/Internal/Http/Http1OutputProducer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

using System;
using System.Buffers;
using System.Collections.Generic;
using System.Diagnostics;
using System.IO.Pipelines;
using System.Threading;
Expand Down Expand Up @@ -42,7 +43,6 @@ public class Http1OutputProducer : IHttpOutputProducer, IHttpOutputAborter, IDis
private long _unflushedBytes;
private bool _autoChunk;
private readonly PipeWriter _pipeWriter;
private const int MemorySizeThreshold = 1024;
private const int BeginChunkLengthMax = 5;
private const int EndChunkLength = 2;

Expand All @@ -56,6 +56,13 @@ public class Http1OutputProducer : IHttpOutputProducer, IHttpOutputAborter, IDis
private bool _currentChunkMemoryUpdated;
private IMemoryOwner<byte> _fakeMemoryOwner;

// Fields needed to store writes before calling either startAsync or Write/FlushAsync
private List<CompletedBuffer> _completedSegments;
private Memory<byte> _currentSegment;
private IMemoryOwner<byte> _currentSegmentOwner;
private int _position;
private bool _startCalled;

public Http1OutputProducer(
PipeWriter pipeWriter,
string connectionId,
Expand Down Expand Up @@ -158,6 +165,10 @@ public Memory<byte> GetMemory(int sizeHint = 0)
{
return GetFakeMemory(sizeHint);
}
else if (!_startCalled)
{
return LeasedMemory(sizeHint);
}
else if (_autoChunk)
{
return GetChunkedMemory(sizeHint);
Expand All @@ -177,6 +188,10 @@ public Span<byte> GetSpan(int sizeHint = 0)
{
return GetFakeMemory(sizeHint).Span;
}
else if (!_startCalled)
{
return LeasedMemory(sizeHint).Span;
}
else if (_autoChunk)
{
return GetChunkedMemory(sizeHint).Span;
Expand All @@ -197,16 +212,23 @@ public void Advance(int bytes)
return;
}

if (_autoChunk)
if (!_startCalled)
{
if (bytes < 0)
if (bytes >= 0)
{
throw new ArgumentOutOfRangeException(nameof(bytes));
}
if (_currentSegment.Length - bytes < _position)
{
throw new ArgumentOutOfRangeException("Can't advance past buffer size.");
}

if (bytes + _advancedBytesForChunk > _currentChunkMemory.Length - BeginChunkLengthMax - EndChunkLength)
_position += bytes;
}
}
else if (_autoChunk)
{
if (_advancedBytesForChunk > _currentChunkMemory.Length - BeginChunkLengthMax - EndChunkLength - bytes)
{
throw new InvalidOperationException("Can't advance past buffer size.");
throw new ArgumentOutOfRangeException("Can't advance past buffer size.");
}
_advancedBytesForChunk += bytes;
}
Expand Down Expand Up @@ -288,8 +310,52 @@ private void WriteResponseHeadersInternal(ref BufferWriter<PipeWriter> writer, i

writer.Commit();

_unflushedBytes += writer.BytesCommitted;
_autoChunk = autoChunk;
WriteDataWrittenBeforeHeaders(ref writer);

_unflushedBytes += writer.BytesCommitted;
_startCalled = true;
}

private void WriteDataWrittenBeforeHeaders(ref BufferWriter<PipeWriter> writer)
{
if (_completedSegments != null)
{
foreach (var segment in _completedSegments)
{
if (_autoChunk)
{
CommitChunkInternal(ref writer, segment.Span);
}
else
{
writer.Write(segment.Span);
writer.Commit();
}
segment.Return();
}

_completedSegments.Clear();
}

if (!_currentSegment.IsEmpty)
{
var segment = _currentSegment.Slice(0, _position);

if (_autoChunk)
{
CommitChunkInternal(ref writer, segment.Span);
}
else
{
writer.Write(segment.Span);
writer.Commit();
}

_position = 0;

DisposeCurrentSegment();
}
}

public void Dispose()
Expand All @@ -302,10 +368,28 @@ public void Dispose()
_fakeMemoryOwner = null;
}

// Call dispose on any memory that wasn't written.
if (_completedSegments != null)
{
foreach (var segment in _completedSegments)
{
segment.Return();
}
}

DisposeCurrentSegment();

CompletePipe();
}
}

private void DisposeCurrentSegment()
{
_currentSegmentOwner?.Dispose();
_currentSegmentOwner = null;
_currentSegment = default;
}

private void CompletePipe()
{
if (!_pipeWriterCompleted)
Expand Down Expand Up @@ -454,7 +538,7 @@ private Memory<byte> GetChunkedMemory(int sizeHint)
}

var memoryMaxLength = _currentChunkMemory.Length - BeginChunkLengthMax - EndChunkLength;
if (_advancedBytesForChunk >= memoryMaxLength - Math.Min(MemorySizeThreshold, sizeHint))
if (_advancedBytesForChunk >= memoryMaxLength - sizeHint && _advancedBytesForChunk > 0)
{
// Chunk is completely written, commit it to the pipe so GetMemory will return a new chunk of memory.
var writer = new BufferWriter<PipeWriter>(_pipeWriter);
Expand Down Expand Up @@ -506,5 +590,90 @@ private Memory<byte> GetFakeMemory(int sizeHint)
}
return _fakeMemoryOwner.Memory;
}

private Memory<byte> LeasedMemory(int sizeHint)
{
EnsureCapacity(sizeHint);
return _currentSegment.Slice(_position);
}

private void EnsureCapacity(int sizeHint)
{
// Only subtracts _position from the current segment length if it's non-null.
// If _currentSegment is null, it returns 0.
var remainingSize = _currentSegment.Length - _position;

// If the sizeHint is 0, any capacity will do
// Otherwise, the buffer must have enough space for the entire size hint, or we need to add a segment.
if ((sizeHint == 0 && remainingSize > 0) || (sizeHint > 0 && remainingSize >= sizeHint))
{
// We have capacity in the current segment
return;
}

AddSegment(sizeHint);
}

private void AddSegment(int sizeHint = 0)
{
if (_currentSegment.Length != 0)
{
// We're adding a segment to the list
if (_completedSegments == null)
{
_completedSegments = new List<CompletedBuffer>();
}

// Position might be less than the segment length if there wasn't enough space to satisfy the sizeHint when
// GetMemory was called. In that case we'll take the current segment and call it "completed", but need to
// ignore any empty space in it.
_completedSegments.Add(new CompletedBuffer(_currentSegmentOwner, _currentSegment, _position));
}

if (sizeHint <= _memoryPool.MaxBufferSize)
{
// Get a new buffer using the minimum segment size, unless the size hint is larger than a single segment.
// Also, the size cannot be larger than the MaxBufferSize of the MemoryPool
var owner = _memoryPool.Rent(Math.Min(sizeHint, _memoryPool.MaxBufferSize));
_currentSegment = owner.Memory;
_currentSegmentOwner = owner;
}
else
{
_currentSegment = new byte[sizeHint];
_currentSegmentOwner = null;
}

_position = 0;
}

/// <summary>
/// Holds a byte[] from the pool and a size value. Basically a Memory but guaranteed to be backed by an ArrayPool byte[], so that we know we can return it.
/// </summary>
private readonly struct CompletedBuffer
{
private readonly IMemoryOwner<byte> _memoryOwner;

public Memory<byte> Buffer { get; }
public int Length { get; }

public ReadOnlySpan<byte> Span => Buffer.Span.Slice(0, Length);

public CompletedBuffer(IMemoryOwner<byte> owner, Memory<byte> buffer, int length)
{
_memoryOwner = owner;

Buffer = buffer;
Length = length;
}

public void Return()
{
if (_memoryOwner != null)
{
_memoryOwner.Dispose();
}
}
}
}
}
42 changes: 26 additions & 16 deletions src/Servers/Kestrel/Core/src/Internal/Http/HttpProtocol.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,11 @@ public abstract partial class HttpProtocol : IDefaultHttpContextContainer, IHttp
// Keep-alive is default for HTTP/1.1 and HTTP/2; parsing and errors will change its value
// volatile, see: https://msdn.microsoft.com/en-us/library/x13ttww7.aspx
protected volatile bool _keepAlive = true;
private bool _canWriteResponseBody;
// _canWriteResponseBody is set in CreateResponseHeaders.
// If we are writing with GetMemory/Advance before calling StartAsync, assume we can write and throw away contents if we can't.
private bool _canWriteResponseBody = true;
private bool _hasAdvanced;
private bool _isLeasedMemoryInvalid = true;
Copy link
Member

Choose a reason for hiding this comment

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

Should _hasAdvanced and _isLeasedMemoryInvalid be reset between requests?

Copy link
Member

Choose a reason for hiding this comment

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

Is there a regression test that will fail if we forgot to reset?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ya caught me.

private bool _autoChunk;
protected Exception _applicationException;
private BadHttpRequestException _requestRejectedException;
Expand Down Expand Up @@ -921,6 +925,8 @@ private void ProduceStart(bool appCompleted)
return;
}

_isLeasedMemoryInvalid = true;

_requestProcessingStatus = RequestProcessingStatus.HeadersCommitted;

var responseHeaders = CreateResponseHeaders(appCompleted);
Expand Down Expand Up @@ -1066,7 +1072,7 @@ private HttpResponseHeaders CreateResponseHeaders(bool appCompleted)
{
_keepAlive = false;
}
else if (appCompleted || !_canWriteResponseBody)
else if ((appCompleted || !_canWriteResponseBody) && !_hasAdvanced) // Avoid setting contentLength of 0 if we wrote data before calling CreateResponseHeaders
Copy link
Member

Choose a reason for hiding this comment

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

if appCompleted && _hasAdvanced then we can set ContentLength to a specific value and avoid chunking.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I was thinking about doing this in this PR but wasn't a fan. You would need to peek into the number of advanced bytes inside of the Http1OutputProducer which isn't clean. I'd prefer to do it in a separate pr.

{
// Don't set the Content-Length header automatically for HEAD requests, 204 responses, or 304 responses.
if (CanAutoSetContentLengthZeroResponseHeader())
Expand Down Expand Up @@ -1268,6 +1274,21 @@ public void ReportApplicationError(Exception ex)

public void Advance(int bytes)
{
if (bytes < 0)
{
throw new ArgumentOutOfRangeException(nameof(bytes));
}
else if (bytes > 0)
{
_hasAdvanced = true;
}

if (_isLeasedMemoryInvalid)
{
throw new InvalidOperationException("Invalid ordering of calling StartAsync and Advance. " +
"Call StartAsync before calling GetMemory/GetSpan and Advance.");
}

if (_canWriteResponseBody)
{
VerifyAndUpdateWrite(bytes);
Expand All @@ -1276,7 +1297,6 @@ public void Advance(int bytes)
else
{
HandleNonBodyResponseWrite();

// For HEAD requests, we still use the number of bytes written for logging
// how many bytes were written.
VerifyAndUpdateWrite(bytes);
Expand All @@ -1285,27 +1305,16 @@ public void Advance(int bytes)

public Memory<byte> GetMemory(int sizeHint = 0)
{
ThrowIfResponseNotStarted();

_isLeasedMemoryInvalid = false;
return Output.GetMemory(sizeHint);
}

public Span<byte> GetSpan(int sizeHint = 0)
{
ThrowIfResponseNotStarted();

_isLeasedMemoryInvalid = false;
return Output.GetSpan(sizeHint);
}

[StackTraceHidden]
private void ThrowIfResponseNotStarted()
{
if (!HasResponseStarted)
{
throw new InvalidOperationException(CoreStrings.StartAsyncBeforeGetMemory);
}
}

public ValueTask<FlushResult> FlushPipeAsync(CancellationToken cancellationToken)
{
if (!HasResponseStarted)
Expand Down Expand Up @@ -1338,6 +1347,7 @@ public void Complete(Exception ex)
ApplicationAbort();
}
}

Output.Complete();
}

Expand Down
1 change: 0 additions & 1 deletion src/Servers/Kestrel/shared/test/TestApp.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,6 @@ public static async Task EchoAppPipeWriter(HttpContext httpContext)
if (buffer.Length > 0)
{
await request.Body.ReadUntilEndAsync(buffer).DefaultTimeout();
await response.StartAsync();
Copy link
Member

@halter73 halter73 Mar 6, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think we should have unrelated tests hit the inefficient code path. Tests like ChunkGetMemoryAndWriteWithoutStart should be sufficient.

var memory = response.BodyWriter.GetMemory(buffer.Length);
buffer.CopyTo(memory);
response.BodyWriter.Advance(buffer.Length);
Expand Down
Loading