Skip to content
This repository was archived by the owner on Dec 18, 2018. It is now read-only.

Commit de5ccb5

Browse files
author
John Luo
committed
PR feedback for #2838 
1 parent 0c3b443 commit de5ccb5

File tree

7 files changed

+61
-61
lines changed

7 files changed

+61
-61
lines changed

‎src/Kestrel.Core/Http2Limits.cs

Lines changed: 5 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -13,11 +13,7 @@ public class Http2Limits
1313
{
1414
private int _maxStreamsPerConnection = 100;
1515
private int _headerTableSize = (int)Http2PeerSettings.DefaultHeaderTableSize;
16-
private int _maxFrameSize = MinAllowedMaxFrameSize;
17-
18-
// These are limits defined by the RFC https://tools.ietf.org/html/rfc7540#section-4.2
19-
public const int MinAllowedMaxFrameSize = 16 * 1024;
20-
public const int MaxAllowedMaxFrameSize = 16 * 1024 * 1024 - 1;
16+
private int _maxFrameSize = (int)Http2PeerSettings.DefaultMaxFrameSize;
2117

2218
/// <summary>
2319
/// Limits the number of concurrent request streams per HTTP/2 connection. Excess streams will be refused.
@@ -42,7 +38,7 @@ public int MaxStreamsPerConnection
4238
/// <summary>
4339
/// Limits the size of the header compression table, in octets, the HPACK decoder on the server can use.
4440
/// <para>
45-
/// Defaults to 4096
41+
/// Value must be greater than 0, defaults to 4096
4642
/// </para>
4743
/// </summary>
4844
public int HeaderTableSize
@@ -62,17 +58,17 @@ public int HeaderTableSize
6258
/// <summary>
6359
/// Indicates the size of the largest frame payload that is allowed to be received, in octets. The size must be between 2^14 and 2^24-1.
6460
/// <para>
65-
/// Defaults to 2^14 (16,384)
61+
/// Value must be between 2^14 and 2^24, defaults to 2^14 (16,384)
6662
/// </para>
6763
/// </summary>
6864
public int MaxFrameSize
6965
{
7066
get => _maxFrameSize;
7167
set
7268
{
73-
if (value < MinAllowedMaxFrameSize || value > MaxAllowedMaxFrameSize)
69+
if (value < Http2PeerSettings.MinAllowedMaxFrameSize || value > Http2PeerSettings.MaxAllowedMaxFrameSize)
7470
{
75-
throw new ArgumentOutOfRangeException(nameof(value), value, CoreStrings.FormatArgumentOutOfRange(MinAllowedMaxFrameSize, MaxAllowedMaxFrameSize));
71+
throw new ArgumentOutOfRangeException(nameof(value), value, CoreStrings.FormatArgumentOutOfRange(Http2PeerSettings.MinAllowedMaxFrameSize, Http2PeerSettings.MaxAllowedMaxFrameSize));
7672
}
7773

7874
_maxFrameSize = value;

‎src/Kestrel.Core/Internal/Http2/Http2Connection.cs

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -606,7 +606,7 @@ private Task ProcessRstStreamFrameAsync()
606606
return Task.CompletedTask;
607607
}
608608

609-
private async Task ProcessSettingsFrameAsync()
609+
private Task ProcessSettingsFrameAsync()
610610
{
611611
if (_currentHeadersStream != null)
612612
{
@@ -625,7 +625,7 @@ private async Task ProcessSettingsFrameAsync()
625625
throw new Http2ConnectionErrorException(CoreStrings.Http2ErrorSettingsAckLengthNotZero, Http2ErrorCode.FRAME_SIZE_ERROR);
626626
}
627627

628-
return;
628+
return Task.CompletedTask;
629629
}
630630

631631
if (_incomingFrame.PayloadLength % 6 != 0)
@@ -642,7 +642,7 @@ private async Task ProcessSettingsFrameAsync()
642642
_clientSettings.Update(_incomingFrame.GetSettings());
643643

644644
// Ack before we update the windows, they could send data immediately.
645-
await _frameWriter.WriteSettingsAckAsync();
645+
var ackTask = _frameWriter.WriteSettingsAckAsync();
646646

647647
if (_clientSettings.MaxFrameSize != previousMaxFrameSize)
648648
{
@@ -665,6 +665,8 @@ private async Task ProcessSettingsFrameAsync()
665665
}
666666
}
667667
}
668+
669+
return ackTask;
668670
}
669671
catch (Http2SettingsParameterOutOfRangeException ex)
670672
{

‎src/Kestrel.Core/Internal/Http2/Http2FrameWriter.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@ public class Http2FrameWriter
2121
// Literal Header Field without Indexing - Indexed Name (Index 8 - :status)
2222
private static readonly byte[] _continueBytes = new byte[] { 0x08, 0x03, (byte)'1', (byte)'0', (byte)'0' };
2323

24-
private uint _maxFrameSize = Http2Limits.MinAllowedMaxFrameSize;
24+
private uint _maxFrameSize = Http2PeerSettings.MinAllowedMaxFrameSize;
2525
private Http2Frame _outgoingFrame;
2626
private readonly object _writeLock = new object();
2727
private readonly HPackEncoder _hpackEncoder = new HPackEncoder();

‎src/Kestrel.Core/Internal/Http2/Http2PeerSettings.cs

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -12,9 +12,11 @@ public class Http2PeerSettings
1212
public const bool DefaultEnablePush = true;
1313
public const uint DefaultMaxConcurrentStreams = uint.MaxValue;
1414
public const uint DefaultInitialWindowSize = 65535;
15-
public const uint DefaultMaxFrameSize = Http2Limits.MinAllowedMaxFrameSize;
15+
public const uint DefaultMaxFrameSize = MinAllowedMaxFrameSize;
1616
public const uint DefaultMaxHeaderListSize = uint.MaxValue;
1717
public const uint MaxWindowSize = int.MaxValue;
18+
internal const int MinAllowedMaxFrameSize = 16 * 1024;
19+
internal const int MaxAllowedMaxFrameSize = 16 * 1024 * 1024 - 1;
1820

1921
public uint HeaderTableSize { get; set; } = DefaultHeaderTableSize;
2022

@@ -64,11 +66,11 @@ public void Update(IList<Http2PeerSetting> settings)
6466
InitialWindowSize = value;
6567
break;
6668
case Http2SettingsParameter.SETTINGS_MAX_FRAME_SIZE:
67-
if (value < Http2Limits.MinAllowedMaxFrameSize || value > Http2Limits.MaxAllowedMaxFrameSize)
69+
if (value < MinAllowedMaxFrameSize || value > MaxAllowedMaxFrameSize)
6870
{
6971
throw new Http2SettingsParameterOutOfRangeException(Http2SettingsParameter.SETTINGS_MAX_FRAME_SIZE,
70-
lowerBound: Http2Limits.MinAllowedMaxFrameSize,
71-
upperBound: Http2Limits.MaxAllowedMaxFrameSize);
72+
lowerBound: MinAllowedMaxFrameSize,
73+
upperBound: MaxAllowedMaxFrameSize);
7274
}
7375

7476
MaxFrameSize = value;

‎test/Kestrel.InMemory.FunctionalTests/Http2/Http2ConnectionTests.cs

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -76,28 +76,28 @@ public class Http2ConnectionTests : Http2TestBase
7676
private static readonly byte[] _worldBytes = Encoding.ASCII.GetBytes("world");
7777
private static readonly byte[] _helloWorldBytes = Encoding.ASCII.GetBytes("hello, world");
7878
private static readonly byte[] _noData = new byte[0];
79-
private static readonly byte[] _maxData = Encoding.ASCII.GetBytes(new string('a', Http2Limits.MinAllowedMaxFrameSize));
79+
private static readonly byte[] _maxData = Encoding.ASCII.GetBytes(new string('a', Http2PeerSettings.MinAllowedMaxFrameSize));
8080

8181
[Fact]
8282
public async Task Frame_Received_OverMaxSize_FrameError()
8383
{
8484
await InitializeConnectionAsync(_echoApplication);
8585

8686
await StartStreamAsync(1, _browserRequestHeaders, endStream: false);
87-
uint length = Http2Limits.MinAllowedMaxFrameSize + 1;
87+
uint length = Http2PeerSettings.MinAllowedMaxFrameSize + 1;
8888
await SendDataAsync(1, new byte[length].AsSpan(), endStream: true);
8989

9090
await WaitForConnectionErrorAsync<Http2ConnectionErrorException>(
9191
ignoreNonGoAwayFrames: true,
9292
expectedLastStreamId: 1,
9393
expectedErrorCode: Http2ErrorCode.FRAME_SIZE_ERROR,
94-
expectedErrorMessage: CoreStrings.FormatHttp2ErrorFrameOverLimit(length, Http2Limits.MinAllowedMaxFrameSize));
94+
expectedErrorMessage: CoreStrings.FormatHttp2ErrorFrameOverLimit(length, Http2PeerSettings.MinAllowedMaxFrameSize));
9595
}
9696

9797
[Fact]
9898
public async Task ServerSettings_ChangesRequestMaxFrameSize()
9999
{
100-
var length = Http2Limits.MinAllowedMaxFrameSize + 10;
100+
var length = Http2PeerSettings.MinAllowedMaxFrameSize + 10;
101101
_connectionContext.ServiceContext.ServerOptions.Limits.Http2.MaxFrameSize = length;
102102
_connection = new Http2Connection(_connectionContext);
103103

@@ -112,11 +112,11 @@ await ExpectAsync(Http2FrameType.HEADERS,
112112
withStreamId: 1);
113113
// The client's settings is still defaulted to Http2PeerSettings.MinAllowedMaxFrameSize so the echo response will come back in two separate frames
114114
await ExpectAsync(Http2FrameType.DATA,
115-
withLength: Http2Limits.MinAllowedMaxFrameSize,
115+
withLength: Http2PeerSettings.MinAllowedMaxFrameSize,
116116
withFlags: (byte)Http2DataFrameFlags.NONE,
117117
withStreamId: 1);
118118
await ExpectAsync(Http2FrameType.DATA,
119-
withLength: length - Http2Limits.MinAllowedMaxFrameSize,
119+
withLength: length - Http2PeerSettings.MinAllowedMaxFrameSize,
120120
withFlags: (byte)Http2DataFrameFlags.NONE,
121121
withStreamId: 1);
122122
await ExpectAsync(Http2FrameType.DATA,
@@ -2139,7 +2139,7 @@ public async Task SETTINGS_ACK_Received_DoesNotSend_ACK()
21392139
{
21402140
await InitializeConnectionAsync(_noopApplication);
21412141

2142-
var frame = new Http2Frame(Http2Limits.MinAllowedMaxFrameSize);
2142+
var frame = new Http2Frame(Http2PeerSettings.MinAllowedMaxFrameSize);
21432143
frame.PrepareSettings(Http2SettingsFrameFlags.ACK);
21442144
await SendAsync(frame.Raw);
21452145

@@ -2262,7 +2262,7 @@ public async Task SETTINGS_Received_ChangesAllowedResponseMaxFrameSize()
22622262
{
22632263
// This includes the default response headers such as :status, etc
22642264
var defaultResponseHeaderLength = 37;
2265-
var headerValueLength = Http2Limits.MinAllowedMaxFrameSize;
2265+
var headerValueLength = Http2PeerSettings.MinAllowedMaxFrameSize;
22662266
// First byte is always 0
22672267
// Second byte is the length of header name which is 1
22682268
// Third byte is the header name which is A/B

0 commit comments

Comments
 (0)