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

Don't send body in HEAD response when using PipeWriter.Advance before headers flushed #59725

Merged
merged 5 commits into from
Mar 6, 2025
Merged

Don't send body in HEAD response when using PipeWriter.Advance before headers flushed #59725

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1a38b96
Don't send body in HEAD response when using PipeWriter.Advance before…
BrennanConroy Jan 6, 2025
9bb37ca
test + fix
BrennanConroy Jan 6, 2025
96c885b
fix state bug
BrennanConroy Jan 14, 2025
000f75d
fb
BrennanConroy Feb 3, 2025
fdf96e3
fb
BrennanConroy Mar 4, 2025
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
45 changes: 27 additions & 18 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 @@ -40,6 +40,7 @@ internal class Http1OutputProducer : IHttpOutputProducer, IDisposable
private bool _aborted;
private long _unflushedBytes;
private int _currentMemoryPrefixBytes;
private bool _canWriteBody = true;

private readonly ConcurrentPipeWriter _pipeWriter;
private IMemoryOwner<byte>? _fakeMemoryOwner;
Expand Down Expand Up @@ -121,7 +122,7 @@ public ValueTask<FlushResult> WriteStreamSuffixAsync()
{
if (!_writeStreamSuffixCalled)
{
if (_autoChunk)
if (_autoChunk && _canWriteBody)
{
var writer = new BufferWriter<PipeWriter>(_pipeWriter);
result = WriteAsyncInternal(ref writer, EndChunkedResponseBytes);
Expand Down Expand Up @@ -333,7 +334,7 @@ private void CommitChunkInternal(ref BufferWriter<PipeWriter> writer, ReadOnlySp
writer.Commit();
}

public void WriteResponseHeaders(int statusCode, string? reasonPhrase, HttpResponseHeaders responseHeaders, bool autoChunk, bool appComplete)
public void WriteResponseHeaders(int statusCode, string? reasonPhrase, HttpResponseHeaders responseHeaders, bool autoChunk, bool appComplete, bool canWriteBody)
{
lock (_contextLock)
{
Expand All @@ -344,6 +345,8 @@ public void WriteResponseHeaders(int statusCode, string? reasonPhrase, HttpRespo
return;
}

_canWriteBody = canWriteBody;

var buffer = _pipeWriter;
var writer = new BufferWriter<PipeWriter>(buffer);
WriteResponseHeadersInternal(ref writer, statusCode, reasonPhrase, responseHeaders, autoChunk);
Expand Down Expand Up @@ -373,14 +376,17 @@ private void WriteDataWrittenBeforeHeaders(ref BufferWriter<PipeWriter> writer)
{
foreach (var segment in _completedSegments)
{
if (_autoChunk)
{
CommitChunkInternal(ref writer, segment.Span);
}
else
if (_canWriteBody)
{
writer.Write(segment.Span);
writer.Commit();
if (_autoChunk)
{
CommitChunkInternal(ref writer, segment.Span);
}
else
{
writer.Write(segment.Span);
writer.Commit();
}
}
segment.Return();
}
Expand All @@ -391,16 +397,19 @@ private void WriteDataWrittenBeforeHeaders(ref BufferWriter<PipeWriter> writer)

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

if (_autoChunk)
{
CommitChunkInternal(ref writer, segment.Span);
}
else
if (_canWriteBody)
{
writer.Write(segment.Span);
writer.Commit();
var segment = _currentSegment.Slice(0, _position);

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

_position = 0;
Expand Down
6 changes: 3 additions & 3 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 @@ -999,7 +999,7 @@ private void ProduceStart(bool appCompleted)

var responseHeaders = CreateResponseHeaders(appCompleted);

Output.WriteResponseHeaders(StatusCode, ReasonPhrase, responseHeaders, _autoChunk, appCompleted);
Output.WriteResponseHeaders(StatusCode, ReasonPhrase, responseHeaders, _autoChunk, appCompleted, canWriteBody: _canWriteResponseBody);
}

private void VerifyInitializeState(int firstWriteByteCount)
Expand Down Expand Up @@ -1645,7 +1645,7 @@ private ValueTask<FlushResult> FirstWriteAsyncInternal(ReadOnlyMemory<byte> data
{
if (data.Length == 0)
{
Output.WriteResponseHeaders(StatusCode, ReasonPhrase, responseHeaders, _autoChunk, appCompleted: false);
Output.WriteResponseHeaders(StatusCode, ReasonPhrase, responseHeaders, _autoChunk, appCompleted: false, canWriteBody: _canWriteResponseBody);
return Output.FlushAsync(cancellationToken);
}

Expand All @@ -1659,7 +1659,7 @@ private ValueTask<FlushResult> FirstWriteAsyncInternal(ReadOnlyMemory<byte> data
}
else
{
Output.WriteResponseHeaders(StatusCode, ReasonPhrase, responseHeaders, _autoChunk, appCompleted: false);
Output.WriteResponseHeaders(StatusCode, ReasonPhrase, responseHeaders, _autoChunk, appCompleted: false, canWriteBody: _canWriteResponseBody);
HandleNonBodyResponseWrite();
return Output.FlushAsync(cancellationToken);
}
Expand Down
2 changes: 1 addition & 1 deletion src/Servers/Kestrel/Core/src/Internal/Http/IHttpOutputProducer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ internal interface IHttpOutputProducer
ValueTask<FlushResult> WriteChunkAsync(ReadOnlySpan<byte> data, CancellationToken cancellationToken);
ValueTask<FlushResult> FlushAsync(CancellationToken cancellationToken);
ValueTask<FlushResult> Write100ContinueAsync();
void WriteResponseHeaders(int statusCode, string? reasonPhrase, HttpResponseHeaders responseHeaders, bool autoChunk, bool appCompleted);
void WriteResponseHeaders(int statusCode, string? reasonPhrase, HttpResponseHeaders responseHeaders, bool autoChunk, bool appCompleted, bool canWriteBody);
// This takes ReadOnlySpan instead of ReadOnlyMemory because it always synchronously copies data before flushing.
ValueTask<FlushResult> WriteDataToPipeAsync(ReadOnlySpan<byte> data, CancellationToken cancellationToken);
// Test hook
Expand Down
6 changes: 4 additions & 2 deletions src/Servers/Kestrel/Core/src/Internal/Http2/Http2OutputProducer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -358,7 +358,8 @@ public ValueTask<FlushResult> Write100ContinueAsync()
}
}

public void WriteResponseHeaders(int statusCode, string? reasonPhrase, HttpResponseHeaders responseHeaders, bool autoChunk, bool appCompleted)
// canWriteBody is ignored as we don't have chunked bodies in HTTP/2 and so writing headers is not affected by canWriteBody
public void WriteResponseHeaders(int statusCode, string? reasonPhrase, HttpResponseHeaders responseHeaders, bool autoChunk, bool appCompleted, bool canWriteBody)
{
lock (_dataWriterLock)
{
Expand Down Expand Up @@ -556,7 +557,8 @@ public ValueTask<FlushResult> FirstWriteAsync(int statusCode, string? reasonPhra
{
lock (_dataWriterLock)
{
WriteResponseHeaders(statusCode, reasonPhrase, responseHeaders, autoChunk, appCompleted: false);
// canWriteBody is hardcoded to true as we don't have chunked bodies in HTTP/2 and so writing headers is not affected by canWriteBody
WriteResponseHeaders(statusCode, reasonPhrase, responseHeaders, autoChunk, appCompleted: false, canWriteBody: true);

return WriteDataToPipeAsync(data, cancellationToken);
}
Expand Down
6 changes: 4 additions & 2 deletions src/Servers/Kestrel/Core/src/Internal/Http3/Http3OutputProducer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,8 @@ public ValueTask<FlushResult> FirstWriteAsync(int statusCode, string? reasonPhra
{
lock (_dataWriterLock)
{
WriteResponseHeaders(statusCode, reasonPhrase, responseHeaders, autoChunk, appCompleted: false);
// canWriteBody is hardcoded to true as we don't have chunked bodies in HTTP/3 and so writing headers is not affected by canWriteBody
WriteResponseHeaders(statusCode, reasonPhrase, responseHeaders, autoChunk, appCompleted: false, canWriteBody: true);

return WriteDataToPipeAsync(data, cancellationToken);
}
Expand Down Expand Up @@ -375,7 +376,8 @@ public ValueTask<FlushResult> WriteDataToPipeAsync(ReadOnlySpan<byte> data, Canc
}
}

public void WriteResponseHeaders(int statusCode, string? reasonPhrase, HttpResponseHeaders responseHeaders, bool autoChunk, bool appCompleted)
// canWriteBody is ignored as we don't have chunked bodies in HTTP/3 and so writing headers is not affected by canWriteBody
public void WriteResponseHeaders(int statusCode, string? reasonPhrase, HttpResponseHeaders responseHeaders, bool autoChunk, bool appCompleted, bool canWriteBody)
{
// appCompleted flag is not used here. The write FIN is sent via the transport and not via the frame.
// Headers are written to buffer and flushed with a FIN when Http3FrameWriter.CompleteAsync is called
Expand Down
149 changes: 147 additions & 2 deletions src/Servers/Kestrel/test/InMemory.FunctionalTests/ResponseTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1426,12 +1426,15 @@ await connection.Receive(
$"Date: {server.Context.DateHeaderValue}",
"",
"");

connection.ShutdownSend();
await connection.ReceiveEnd();
}
}
}

[Fact]
public async Task HeadResponseBodyNotWrittenWithAsyncWrite()
public async Task HeadResponseHeadersWrittenWithAsyncWriteBeforeAppCompletes()
Copy link
Member Author

Choose a reason for hiding this comment

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

aspnet/KestrelHttpServer#1204 changed some of the HEAD response tests to only check the response headers are flushed and doesn't fully check that the body doesn't exist. So split the test into two for the flushed headers check, and the lack of body check.

{
var flushed = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously);

Expand Down Expand Up @@ -1461,8 +1464,68 @@ await connection.Receive(
}
}

[Fact]
public async Task HeadResponseBodyNotWrittenWithAsyncWrite()
{
await using (var server = new TestServer(async httpContext =>
{
httpContext.Response.ContentLength = 12;
await httpContext.Response.WriteAsync("hello, world");
}, new TestServiceContext(LoggerFactory)))
{
using (var connection = server.CreateConnection())
{
await connection.Send(
"HEAD / HTTP/1.1",
"Host:",
"",
"");
await connection.Receive(
"HTTP/1.1 200 OK",
"Content-Length: 12",
$"Date: {server.Context.DateHeaderValue}",
"",
"");

connection.ShutdownSend();
await connection.ReceiveEnd();
}
}
}

[Fact]
public async Task HeadResponseBodyNotWrittenWithSyncWrite()
{
var serviceContext = new TestServiceContext(LoggerFactory) { ServerOptions = { AllowSynchronousIO = true } };

await using (var server = new TestServer(httpContext =>
{
httpContext.Response.ContentLength = 12;
httpContext.Response.Body.Write(Encoding.ASCII.GetBytes("hello, world"), 0, 12);
return Task.CompletedTask;
}, serviceContext))
{
using (var connection = server.CreateConnection())
{
await connection.Send(
"HEAD / HTTP/1.1",
"Host:",
"",
"");
await connection.Receive(
"HTTP/1.1 200 OK",
"Content-Length: 12",
$"Date: {server.Context.DateHeaderValue}",
"",
"");
connection.ShutdownSend();
await connection.ReceiveEnd();
}
}
}

[Fact]
public async Task HeadResponseHeadersWrittenWithSyncWriteBeforeAppCompletes()
{
var flushed = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously);

Expand All @@ -1471,7 +1534,7 @@ public async Task HeadResponseBodyNotWrittenWithSyncWrite()
await using (var server = new TestServer(async httpContext =>
{
httpContext.Response.ContentLength = 12;
await httpContext.Response.BodyWriter.WriteAsync(new Memory<byte>(Encoding.ASCII.GetBytes("hello, world"), 0, 12));
httpContext.Response.Body.Write(Encoding.ASCII.GetBytes("hello, world"), 0, 12);
Copy link
Member Author

Choose a reason for hiding this comment

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

#7110 removed the sync write which this test and HeadResponseBodyNotWrittenWithSyncWrite above were explicitly testing.

await flushed.Task;
}, serviceContext))
{
Expand All @@ -1494,6 +1557,88 @@ await connection.Receive(
}
}

[Fact]
public async Task HeadResponseBodyNotWrittenWithAdvanceBeforeFlush()
{
var serviceContext = new TestServiceContext(LoggerFactory) { ServerOptions = { AllowSynchronousIO = true } };

await using (var server = new TestServer(async httpContext =>
{
var span = httpContext.Response.BodyWriter.GetSpan(5);
for (var i = 0; i < span.Length; i++)
{
span[i] = (byte)'h';
}
httpContext.Response.BodyWriter.Advance(span.Length);
await httpContext.Response.BodyWriter.FlushAsync();
}, serviceContext))
{
using (var connection = server.CreateConnection())
{
await connection.Send(
"HEAD / HTTP/1.1",
"Host:",
"",
"");
await connection.Receive(
"HTTP/1.1 200 OK",
$"Date: {server.Context.DateHeaderValue}",
"Transfer-Encoding: chunked",
"",
"");

connection.ShutdownSend();
await connection.ReceiveEnd();
}
}
}

[Fact]
public async Task HeadResponseBodyNotWrittenWithAdvanceBeforeAndAfterFlush()
{
var serviceContext = new TestServiceContext(LoggerFactory) { ServerOptions = { AllowSynchronousIO = true } };

await using (var server = new TestServer(async httpContext =>
{
// Make response chunked
var span = httpContext.Response.BodyWriter.GetSpan(5);
for (var i = 0; i < span.Length; i++)
{
span[i] = (byte)'h';
}
httpContext.Response.BodyWriter.Advance(span.Length);
await httpContext.Response.BodyWriter.FlushAsync();

// Send after headers flushed
span = httpContext.Response.BodyWriter.GetSpan(5);
for (var i = 0; i < span.Length; i++)
{
span[i] = (byte)'h';
}
httpContext.Response.BodyWriter.Advance(span.Length);
await httpContext.Response.BodyWriter.FlushAsync();
}, serviceContext))
{
using (var connection = server.CreateConnection())
{
await connection.Send(
"HEAD / HTTP/1.1",
"Host:",
"",
"");
await connection.Receive(
"HTTP/1.1 200 OK",
$"Date: {server.Context.DateHeaderValue}",
"Transfer-Encoding: chunked",
"",
"");

connection.ShutdownSend();
await connection.ReceiveEnd();
}
}
}

[Fact]
public async Task ZeroLengthWritesFlushHeaders()
{
Expand Down
Loading