Skip to content

Disable request body data rate limits per HTTP/2 stream #10355

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.

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 2 commits into from
May 18, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 3 additions & 0 deletions src/Servers/Kestrel/Core/src/CoreStrings.resx
Original file line number Diff line number Diff line change
Expand Up @@ -599,4 +599,7 @@ For more information on configuring HTTPS see https://go.microsoft.com/fwlink/?l
<data name="StartAsyncBeforeGetMemory" xml:space="preserve">
<value>Cannot call GetMemory() until response has started. Call HttpResponse.StartAsync() before calling GetMemory().</value>
</data>
<data name="Http2MinDataRateNotSupported" xml:space="preserve">
<value>This feature is not supported for HTTP/2 requests except to disable it entirely by setting the rate to null.</value>
</data>
</root>
Original file line number Diff line number Diff line change
Expand Up @@ -5,17 +5,17 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Features
{
/// <summary>
/// Feature to set the minimum data rate at which the the request body must be sent by the client.
/// This feature is not available for HTTP/2 requests. Instead, use <see cref="KestrelServerLimits.MinRequestBodyDataRate"/>
/// for server-wide configuration which applies to both HTTP/2 and HTTP/1.x.
/// This feature is not supported for HTTP/2 requests except to disable it entirely by setting <see cref="MinDataRate"/> to <see langword="null"/>
/// Instead, use <see cref="KestrelServerLimits.MinRequestBodyDataRate"/> for server-wide configuration which applies to both HTTP/2 and HTTP/1.x.
/// </summary>
public interface IHttpMinRequestBodyDataRateFeature
{
/// <summary>
/// The minimum data rate in bytes/second at which the request body must be sent by the client.
/// Setting this property to null indicates no minimum data rate should be enforced.
/// This limit has no effect on upgraded connections which are always unlimited.
/// This feature is not available for HTTP/2 requests. Instead, use <see cref="KestrelServerLimits.MinRequestBodyDataRate"/>
/// for server-wide configuration which applies to both HTTP/2 and HTTP/1.x.
/// This feature is not supported for HTTP/2 requests except to disable it entirely by setting <see cref="MinDataRate"/> to <see langword="null"/>
/// Instead, use <see cref="KestrelServerLimits.MinRequestBodyDataRate"/> for server-wide configuration which applies to both HTTP/2 and HTTP/1.x.
/// </summary>
MinDataRate MinDataRate { get; set; }
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,18 +1,12 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
using System.IO;
using System.Threading;
using System.Threading.Tasks;
using Microsoft.AspNetCore.Http;
using Microsoft.AspNetCore.Http.Features;
using Microsoft.AspNetCore.Server.Kestrel.Core.Features;

namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http
{
internal partial class Http1Connection : IHttpMinRequestBodyDataRateFeature,
IHttpMinResponseDataRateFeature
IHttpMinResponseDataRateFeature
{
MinDataRate IHttpMinRequestBodyDataRateFeature.MinDataRate
{
Expand Down
3 changes: 0 additions & 3 deletions src/Servers/Kestrel/Core/src/Internal/Http/Http1Connection.cs
Original file line number Diff line number Diff line change
Expand Up @@ -62,8 +62,6 @@ public Http1Connection(HttpConnectionContext context)

public bool RequestTimedOut => _requestTimedOut;

public MinDataRate MinRequestBodyDataRate { get; set; }

public MinDataRate MinResponseDataRate { get; set; }

public MemoryPool<byte> MemoryPool { get; }
Expand Down Expand Up @@ -542,7 +540,6 @@ protected override void OnReset()
_remainingRequestHeadersBytesAllowed = ServerOptions.Limits.MaxRequestHeadersTotalSize + 2;
_requestCount++;

MinRequestBodyDataRate = ServerOptions.Limits.MinRequestBodyDataRate;
MinResponseDataRate = ServerOptions.Limits.MinResponseDataRate;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ internal abstract class Http1MessageBody : MessageBody
protected readonly Http1Connection _context;

protected Http1MessageBody(Http1Connection context)
: base(context, context.MinRequestBodyDataRate)
: base(context)
{
_context = context;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,7 @@ private void FastReset()
_currentIHttpRequestLifetimeFeature = this;
_currentIHttpConnectionFeature = this;
_currentIHttpMaxRequestBodySizeFeature = this;
_currentIHttpMinRequestBodyDataRateFeature = this;
_currentIHttpBodyControlFeature = this;
_currentIHttpResponseStartFeature = this;
_currentIRouteValuesFeature = this;
Expand All @@ -100,7 +101,6 @@ private void FastReset()
_currentITlsConnectionFeature = null;
_currentIHttpWebSocketFeature = null;
_currentISessionFeature = null;
_currentIHttpMinRequestBodyDataRateFeature = null;
_currentIHttpMinResponseDataRateFeature = null;
_currentIHttpSendFileFeature = null;
}
Expand Down
2 changes: 2 additions & 0 deletions src/Servers/Kestrel/Core/src/Internal/Http/HttpProtocol.cs
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,7 @@ public HttpProtocol(HttpConnectionContext context)
public string ConnectionIdFeature { get; set; }
public bool HasStartedConsumingRequestBody { get; set; }
public long? MaxRequestBodySize { get; set; }
public MinDataRate MinRequestBodyDataRate { get; set; }
public bool AllowSynchronousIO { get; set; }

/// <summary>
Expand Down Expand Up @@ -340,6 +341,7 @@ public void Reset()

HasStartedConsumingRequestBody = false;
MaxRequestBodySize = ServerOptions.Limits.MaxRequestBodySize;
MinRequestBodyDataRate = ServerOptions.Limits.MinRequestBodyDataRate;
AllowSynchronousIO = ServerOptions.AllowSynchronousIO;
TraceIdentifier = null;
Method = HttpMethod.None;
Expand Down
8 changes: 3 additions & 5 deletions src/Servers/Kestrel/Core/src/Internal/Http/MessageBody.cs
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ internal abstract class MessageBody
private static readonly MessageBody _zeroContentLengthKeepAlive = new ZeroContentLengthMessageBody(keepAlive: true);

private readonly HttpProtocol _context;
private readonly MinDataRate _minRequestBodyDataRate;

private bool _send100Continue = true;
private long _consumedBytes;
Expand All @@ -25,10 +24,9 @@ internal abstract class MessageBody
protected bool _backpressure;
protected long _alreadyTimedBytes;

protected MessageBody(HttpProtocol context, MinDataRate minRequestBodyDataRate)
protected MessageBody(HttpProtocol context)
{
_context = context;
_minRequestBodyDataRate = minRequestBodyDataRate;
}

public static MessageBody ZeroContentLengthClose => _zeroContentLengthClose;
Expand Down Expand Up @@ -98,10 +96,10 @@ protected void TryStart()
{
Log.RequestBodyStart(_context.ConnectionIdFeature, _context.TraceIdentifier);

if (_minRequestBodyDataRate != null)
if (_context.MinRequestBodyDataRate != null)
{
_timingEnabled = true;
_context.TimeoutControl.StartRequestBody(_minRequestBodyDataRate);
_context.TimeoutControl.StartRequestBody(_context.MinRequestBodyDataRate);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http
internal sealed class ZeroContentLengthMessageBody : MessageBody
{
public ZeroContentLengthMessageBody(bool keepAlive)
: base(null, null)
: base(null)
{
RequestKeepAlive = keepAlive;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,8 @@ internal sealed class Http2MessageBody : MessageBody
private ReadResult _readResult;
private long _alreadyExaminedInNextReadResult;

private Http2MessageBody(Http2Stream context, MinDataRate minRequestBodyDataRate)
: base(context, minRequestBodyDataRate)
private Http2MessageBody(Http2Stream context)
: base(context)
{
_context = context;
}
Expand Down Expand Up @@ -47,14 +47,14 @@ protected override void OnDataRead(long bytesRead)
AddAndCheckConsumedBytes(bytesRead);
}

public static MessageBody For(Http2Stream context, MinDataRate minRequestBodyDataRate)
public static MessageBody For(Http2Stream context)
{
if (context.ReceivedEmptyRequestBody)
{
return ZeroContentLengthClose;
}

return new Http2MessageBody(context, minRequestBodyDataRate);
return new Http2MessageBody(context);
}

public override void AdvanceTo(SequencePosition consumed)
Expand Down
Original file line number Diff line number Diff line change
@@ -1,14 +1,18 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
using Microsoft.AspNetCore.Http;
using Microsoft.AspNetCore.Http.Features;
using Microsoft.AspNetCore.Server.Kestrel.Core.Features;
using Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http;

namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2
{
internal partial class Http2Stream : IHttp2StreamIdFeature, IHttpResponseTrailersFeature
internal partial class Http2Stream : IHttp2StreamIdFeature,
IHttpMinRequestBodyDataRateFeature,
IHttpResponseTrailersFeature

{
internal HttpResponseTrailers Trailers { get; set; }
private IHeaderDictionary _userTrailers;
Expand All @@ -30,5 +34,19 @@ IHeaderDictionary IHttpResponseTrailersFeature.Trailers
}

int IHttp2StreamIdFeature.StreamId => _context.StreamId;

MinDataRate IHttpMinRequestBodyDataRateFeature.MinDataRate
{
get => throw new NotSupportedException(CoreStrings.Http2MinDataRateNotSupported);
Copy link
Member

@Tratcher Tratcher May 17, 2019

Choose a reason for hiding this comment

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

The getter should return null if that's the only allowed value, or it should at least return null if already set to null. Throwing getters are no fun.

Copy link
Member Author

@halter73 halter73 May 17, 2019

Choose a reason for hiding this comment

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

The thing is we generally are enforcing a rate limit for HTTP/2. It's just per connection, not per stream. So I don't think it makes sense to return null, but I also don't think it makes sense to return the entire connection's rate limit either.

set
{
if (value != null)
{
throw new NotSupportedException(CoreStrings.Http2MinDataRateNotSupported);
}

MinRequestBodyDataRate = value;
}
}
}
}
2 changes: 1 addition & 1 deletion src/Servers/Kestrel/Core/src/Internal/Http2/Http2Stream.cs
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ protected override string CreateRequestId()
=> StringUtilities.ConcatAsHexSuffix(ConnectionId, ':', (uint)StreamId);

protected override MessageBody CreateMessageBody()
=> Http2MessageBody.For(this, ServerOptions.Limits.MinRequestBodyDataRate);
=> Http2MessageBody.For(this);

// Compare to Http1Connection.OnStartLine
protected override bool TryParseRequest(ReadResult result, out bool endConnection)
Expand Down
14 changes: 14 additions & 0 deletions src/Servers/Kestrel/Core/src/Properties/CoreStrings.Designer.cs

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion src/Servers/Kestrel/Core/test/BodyControlTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,7 @@ public async Task ResponsePipeThrowsObjectDisposedExceptionAfterStop()
private class MockMessageBody : MessageBody
{
public MockMessageBody(bool upgradeable = false)
: base(null, null)
: base(null)
{
RequestUpgrade = upgradeable;
}
Expand Down
2 changes: 1 addition & 1 deletion src/Servers/Kestrel/Core/test/Http1ConnectionTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -888,7 +888,7 @@ public async Task ConsumesRequestWhenApplicationDoesNotConsumeIt()
var buffer = new byte[10];
await context.Response.Body.WriteAsync(buffer, 0, 10);
});
var mockMessageBody = new Mock<MessageBody>(null, null);
var mockMessageBody = new Mock<MessageBody>(null);
_http1Connection.NextMessageBody = mockMessageBody.Object;

var requestProcessingTask = _http1Connection.ProcessRequestsAsync(httpApplication);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -159,12 +159,9 @@ public void FeaturesSetByGenericSameAsByType()
}

[Fact]
public void Http2StreamFeatureCollectionDoesNotIncludeMinRateFeatures()
public void Http2StreamFeatureCollectionDoesNotIncludeIHttpMinResponseDataRateFeature()
{
Assert.Null(_http2Collection.Get<IHttpMinRequestBodyDataRateFeature>());
Assert.Null(_http2Collection.Get<IHttpMinResponseDataRateFeature>());

Assert.NotNull(_collection.Get<IHttpMinRequestBodyDataRateFeature>());
Assert.NotNull(_collection.Get<IHttpMinResponseDataRateFeature>());
}

Expand All @@ -177,6 +174,23 @@ public void Http2StreamFeatureCollectionDoesIncludeUpgradeFeature()
Assert.False(upgradeFeature.IsUpgradableRequest);
}

[Fact]
public void Http2StreamFeatureCollectionDoesIncludeIHttpMinRequestBodyDataRateFeature()
{
var minRateFeature = _http2Collection.Get<IHttpMinRequestBodyDataRateFeature>();

Assert.NotNull(minRateFeature);

Assert.Throws<NotSupportedException>(() => minRateFeature.MinDataRate);
Assert.Throws<NotSupportedException>(() => minRateFeature.MinDataRate = new MinDataRate(1, TimeSpan.FromSeconds(2)));

// You can set the MinDataRate to null though.
minRateFeature.MinDataRate = null;

// But you still cannot read the property;
Assert.Throws<NotSupportedException>(() => minRateFeature.MinDataRate);
}

private void CompareGenericGetterToIndexer()
{
Assert.Same(_collection.Get<IHttpRequestFeature>(), _collection[typeof(IHttpRequestFeature)]);
Expand Down
2 changes: 1 addition & 1 deletion src/Servers/Kestrel/Core/test/HttpRequestStreamTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ public async Task SynchronousReadsThrowIfDisallowedByIHttpBodyControlFeature()

var mockBodyControl = new Mock<IHttpBodyControlFeature>();
mockBodyControl.Setup(m => m.AllowSynchronousIO).Returns(() => allowSynchronousIO);
var mockMessageBody = new Mock<MessageBody>(null, null);
var mockMessageBody = new Mock<MessageBody>(null);
mockMessageBody.Setup(m => m.ReadAsync(CancellationToken.None)).Returns(new ValueTask<ReadResult>(new ReadResult(default, isCanceled: false, isCompleted: true)));

var pipeReader = new HttpRequestPipeReader();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -798,6 +798,60 @@ await WaitForConnectionErrorAsync<ConnectionAbortedException>(
_mockConnectionContext.VerifyNoOtherCalls();
}

[Fact]
public async Task DATA_Received_SlowlyWhenRateLimitDisabledPerRequest_DoesNotAbortConnection()
{
var mockSystemClock = _serviceContext.MockSystemClock;
var limits = _serviceContext.ServerOptions.Limits;

// Use non-default value to ensure the min request and response rates aren't mixed up.
limits.MinRequestBodyDataRate = new MinDataRate(480, TimeSpan.FromSeconds(2.5));

_timeoutControl.Initialize(mockSystemClock.UtcNow.Ticks);

await InitializeConnectionAsync(context =>
{
// Completely disable rate limiting for this stream.
context.Features.Get<IHttpMinRequestBodyDataRateFeature>().MinDataRate = null;
return _readRateApplication(context);
});

// _helloWorldBytes is 12 bytes, and 12 bytes / 240 bytes/sec = .05 secs which is far below the grace period.
await StartStreamAsync(1, ReadRateRequestHeaders(_helloWorldBytes.Length), endStream: false);
await SendDataAsync(1, _helloWorldBytes, endStream: false);

await ExpectAsync(Http2FrameType.HEADERS,
withLength: 37,
withFlags: (byte)Http2HeadersFrameFlags.END_HEADERS,
withStreamId: 1);

await ExpectAsync(Http2FrameType.DATA,
withLength: 1,
withFlags: (byte)Http2DataFrameFlags.NONE,
withStreamId: 1);

// Don't send any more data and advance just to and then past the grace period.
AdvanceClock(limits.MinRequestBodyDataRate.GracePeriod);

_mockTimeoutHandler.Verify(h => h.OnTimeout(It.IsAny<TimeoutReason>()), Times.Never);

AdvanceClock(TimeSpan.FromTicks(1));

_mockTimeoutHandler.Verify(h => h.OnTimeout(It.IsAny<TimeoutReason>()), Times.Never);

await SendDataAsync(1, _helloWorldBytes, endStream: true);

await ExpectAsync(Http2FrameType.DATA,
withLength: 0,
withFlags: (byte)Http2DataFrameFlags.END_STREAM,
withStreamId: 1);

await StopConnectionAsync(expectedLastStreamId: 1, ignoreNonGoAwayFrames: false);

_mockTimeoutHandler.VerifyNoOtherCalls();
_mockConnectionContext.VerifyNoOtherCalls();
}

[Fact]
public async Task DATA_Received_SlowlyDueToConnectionFlowControl_DoesNotAbortConnection()
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ public static string GenerateFile()
"IHttpRequestLifetimeFeature",
"IHttpConnectionFeature",
"IHttpMaxRequestBodySizeFeature",
"IHttpMinRequestBodyDataRateFeature",
"IHttpBodyControlFeature",
"IHttpResponseStartFeature",
"IRouteValuesFeature",
Expand Down