-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
HTTP2 Dynamic Window (prototype/early draft) #52862
Changes from 33 commits
803f47d
2b8922d
93b691b
6a86867
0767cc4
9a3668c
2949234
de6274f
67ee0a5
791ad96
1a79274
519fbac
ff533a5
81c38e0
b60489c
037484a
8578ba7
7ed17f7
869577b
74df445
6e931cd
f0a2992
623dfe9
a9591fb
acc64c5
1c655c3
7d46cdb
0e4040b
e2d002f
1409ad5
da86afa
8739547
57ade94
dd2e982
90dce09
a8bd26a
4e521e5
9bef28c
aa256aa
48cf3d2
ec2d81a
cd73395
9df8166
b34df9f
1645b39
8355c75
e503e25
461291c
0e17349
869c377
3ddd788
d1cd6d8
197d962
85e70ca
3b95797
9e2a68f
0a27183
03e154f
c01860b
72f290b
0683044
ee375ca
ca19ab4
56700dd
66914db
144c47f
d24d332
13a5322
fe053df
763f368
fbdfc29
00fb8a9
1b48ac7
5c59fe7
dab35ab
da9656a
a692cf9
c48e340
3ef586c
8a349af
4a0d057
a5c95b7
ed18c5b
ce1920a
f30e6b1
86e1d68
ebb80a9
0afed46
79024f1
bf15458
d42194c
6e0dc5e
b02415f
4b9d7f2
9a86ec9
25f7757
37ee6f9
145ace9
3776806
8b16b7c
511bf56
49841af
b75827c
5e388f4
1050b15
e03d3a1
857b8ac
a3f0dde
03d70ca
0f9f065
0485d63
bd1b177
201641e
071676a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,55 @@ | ||
// Licensed to the .NET Foundation under one or more agreements. | ||
// The .NET Foundation licenses this file to you under the MIT license. | ||
|
||
using System; | ||
using System.Net.Sockets; | ||
using System.Runtime.InteropServices; | ||
|
||
internal static partial class Interop | ||
{ | ||
internal static partial class Winsock | ||
{ | ||
private const int SioTcpInfo = unchecked((int)3623878695L); | ||
|
||
[DllImport(Interop.Libraries.Ws2_32, SetLastError = true, EntryPoint = "WSAIoctl")] | ||
private static extern SocketError WSAIoctl_Blocking( | ||
SafeSocketHandle socketHandle, | ||
[In] int ioControlCode, | ||
[In] ref int inBuffer, | ||
[In] int inBufferSize, | ||
[Out] out _TCP_INFO_v0 outBuffer, | ||
[In] int outBufferSize, | ||
[Out] out int bytesTransferred, | ||
[In] IntPtr overlapped, | ||
[In] IntPtr completionRoutine); | ||
|
||
internal static unsafe SocketError GetTcpInfoV0(SafeSocketHandle socketHandle, out _TCP_INFO_v0 tcpInfo) | ||
{ | ||
int input = 0; | ||
return WSAIoctl_Blocking(socketHandle, SioTcpInfo, ref input, sizeof(int), out tcpInfo, sizeof(_TCP_INFO_v0), out _, IntPtr.Zero, IntPtr.Zero); | ||
} | ||
|
||
internal struct _TCP_INFO_v0 | ||
{ | ||
internal System.Net.NetworkInformation.TcpState State; | ||
internal uint Mss; | ||
internal ulong ConnectionTimeMs; | ||
internal byte TimestampsEnabled; | ||
internal uint RttUs; | ||
internal uint MinRttUs; | ||
internal uint BytesInFlight; | ||
internal uint Cwnd; | ||
internal uint SndWnd; | ||
internal uint RcvWnd; | ||
internal uint RcvBuf; | ||
internal ulong BytesOut; | ||
internal ulong BytesIn; | ||
internal uint BytesReordered; | ||
internal uint BytesRetrans; | ||
internal uint FastRetrans; | ||
internal uint DupAcksIn; | ||
internal uint TimeoutEpisodes; | ||
internal byte SynRetrans; | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -38,6 +38,7 @@ internal sealed partial class Http2Connection : HttpConnectionBase, IDisposable | |
|
||
private readonly CreditManager _connectionWindow; | ||
private readonly CreditManager _concurrentStreams; | ||
private readonly RttEstimator? _rttEstimator; | ||
|
||
private int _nextStream; | ||
private bool _expectingSettingsAck; | ||
|
@@ -79,21 +80,26 @@ internal sealed partial class Http2Connection : HttpConnectionBase, IDisposable | |
#else | ||
private const int InitialConnectionBufferSize = 4096; | ||
#endif | ||
|
||
private const int DefaultInitialWindowSize = 65535; | ||
// According to https://httpwg.org/specs/rfc7540.html#rfc.section.6.9.2: | ||
// "The connection flow-control window can only be changed using WINDOW_UPDATE frames." | ||
// We need to initialize _connectionWindow with the value 65535, since | ||
// higher values may violate the protocol, and lead to overflows, | ||
// when the server (or an intermediate proxy) sens a very high connection WINDOW_UPDATE increment. | ||
private const int DefaultInitialConnectionWindowSize = 65535; | ||
|
||
// We don't really care about limiting control flow at the connection level. | ||
// We limit it per stream, and the user controls how many streams are created. | ||
// So set the connection window size to a large value. | ||
private const int ConnectionWindowSize = 64 * 1024 * 1024; | ||
private const int ConnectionWindowUpdateRatio = 8; | ||
|
||
// We hold off on sending WINDOW_UPDATE until we hit thi minimum threshold. | ||
// This value is somewhat arbitrary; the intent is to ensure it is much smaller than | ||
// the window size itself, or we risk stalling the server because it runs out of window space. | ||
// If we want to further reduce the frequency of WINDOW_UPDATEs, it's probably better to | ||
// increase the window size (and thus increase the threshold proportionally) | ||
// rather than just increase the threshold. | ||
private const int ConnectionWindowThreshold = ConnectionWindowSize / 8; | ||
private const int ConnectionWindowThreshold = ConnectionWindowSize / ConnectionWindowUpdateRatio; | ||
|
||
// When buffering outgoing writes, we will automatically buffer up to this number of bytes. | ||
// Single writes that are larger than the buffer can cause the buffer to expand beyond | ||
|
@@ -117,7 +123,7 @@ internal enum KeepAliveState | |
private long _keepAlivePingTimeoutTimestamp; | ||
private volatile KeepAliveState _keepAliveState; | ||
|
||
public Http2Connection(HttpConnectionPool pool, Stream stream) | ||
public Http2Connection(HttpConnectionPool pool, Stream stream, System.Net.Sockets.Socket? socket) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We don't always have a socket. What do we do if we don't have a socket? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we don't have a If this assumption does not work for some reason, we'll have to introduce an abstraction for providing RTT estimations. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think that's a reasonable assumption in general. It seems like another option would be to try to measure RTT ourselves, e.g. by detecting how long it takes to ACK a SETTINGS frame or a PING. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That would require a bunch of extra code and changes. To get an accurate estimation, we should send out the PINGs multiple times, on a regular basis. We'll basically end up duplicating the corresponding logic from the TCP stack. (And note that TCP can rely on ACK, and doesn't have to do extra pings) Unless it blocks some important scenarios, I don't think it's worth to do it now, I would rather handle it as an improvement opportunity for future. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you think that's easier and/or provides better data, then I'm fine with that. That said, (a) We need to work reasonably well for cases where we don't have access to a socket. A very common case here is just a wrapper stream on top of NetworkStream/SslStream that's used for logging or diagnostics, or just for initial connection creation, etc. Ideally this should work just as well as if you don't use ConnectCallback etc. It's a really crappy customer story to say "hey we have this cool ConnectCallback, but it will cause a big perf hit in certain cases". We already fixed one major issue here in 6.0 with idle connection polling. Whenever possible, we should strive to not assume we are running on top of a Socket. (b) Getting the RTT times is plat-specific and thus we're going to end up having to do this for Windows, Linux, and MacOS separately. That looks like a fair amount of work, and I'm not sure it's any easier to do all that vs just do our own RTT estimate. (c) It's not clear to me how accurate we need the RTT estimate to be. Mostly we just want to grow the buffer sufficiently so it doesn't become a bottleneck. If we err on the side of buffering too much, that's probably not a huge problem, as compared to buffering too little. |
||
{ | ||
_pool = pool; | ||
_stream = stream; | ||
|
@@ -128,13 +134,17 @@ public Http2Connection(HttpConnectionPool pool, Stream stream) | |
|
||
_httpStreams = new Dictionary<int, Http2Stream>(); | ||
|
||
_connectionWindow = new CreditManager(this, nameof(_connectionWindow), DefaultInitialWindowSize); | ||
_connectionWindow = new CreditManager(this, nameof(_connectionWindow), DefaultInitialConnectionWindowSize); | ||
_concurrentStreams = new CreditManager(this, nameof(_concurrentStreams), InitialMaxConcurrentStreams); | ||
InitialStreamWindowSize = pool.Settings._initialStreamWindowSize; | ||
_rttEstimator = _pool.Settings._fakeRtt != null || socket != null ? | ||
new RttEstimator(this, _pool.Settings._fakeRtt, socket) : | ||
null; | ||
|
||
_writeChannel = Channel.CreateUnbounded<WriteQueueEntry>(s_channelOptions); | ||
|
||
_nextStream = 1; | ||
_initialWindowSize = DefaultInitialWindowSize; | ||
_initialWindowSize = DefaultInitialConnectionWindowSize; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is weird now that you've change the name of the constant to DefaultInitialConnectionWindowSize. _initialWindowSize tracks the current initial window size for streams. I think we should either leave the naming as-is, or define two separate constants, one for connection and one for stream. I would suggest |
||
|
||
_maxConcurrentStreams = InitialMaxConcurrentStreams; | ||
_pendingWindowUpdate = 0; | ||
|
@@ -178,18 +188,24 @@ public async ValueTask SetupAsync() | |
s_http2ConnectionPreface.AsSpan().CopyTo(_outgoingBuffer.AvailableSpan); | ||
_outgoingBuffer.Commit(s_http2ConnectionPreface.Length); | ||
|
||
// Send SETTINGS frame. Disable push promise. | ||
FrameHeader.WriteTo(_outgoingBuffer.AvailableSpan, FrameHeader.SettingLength, FrameType.Settings, FrameFlags.None, streamId: 0); | ||
// Send SETTINGS frame. Disable push promise & set initial window size. | ||
FrameHeader.WriteTo(_outgoingBuffer.AvailableSpan, 2*FrameHeader.SettingLength, FrameType.Settings, FrameFlags.None, streamId: 0); | ||
_outgoingBuffer.Commit(FrameHeader.Size); | ||
BinaryPrimitives.WriteUInt16BigEndian(_outgoingBuffer.AvailableSpan, (ushort)SettingId.EnablePush); | ||
_outgoingBuffer.Commit(2); | ||
BinaryPrimitives.WriteUInt32BigEndian(_outgoingBuffer.AvailableSpan, 0); | ||
_outgoingBuffer.Commit(4); | ||
BinaryPrimitives.WriteUInt16BigEndian(_outgoingBuffer.AvailableSpan, (ushort)SettingId.InitialWindowSize); | ||
_outgoingBuffer.Commit(2); | ||
BinaryPrimitives.WriteUInt32BigEndian(_outgoingBuffer.AvailableSpan, (uint)InitialStreamWindowSize); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's confusing that we now have two very similarly named things: _initialWindowSize -- this is the value from the peer, for outbound streams If we plan on keeping the latter, we should rename these for clarity. |
||
_outgoingBuffer.Commit(4); | ||
|
||
// Send initial connection-level WINDOW_UPDATE | ||
uint windowUpdateAmount = (uint)(ConnectionWindowSize - InitialStreamWindowSize); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this using InitialStreamWindowSize instead of DefaultInitialConnectionWindowSize? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point, will change. |
||
if (NetEventSource.Log.IsEnabled()) Trace($"Initial connection-level WINDOW_UPDATE, windowUpdateAmount={windowUpdateAmount}"); | ||
FrameHeader.WriteTo(_outgoingBuffer.AvailableSpan, FrameHeader.WindowUpdateLength, FrameType.WindowUpdate, FrameFlags.None, streamId: 0); | ||
_outgoingBuffer.Commit(FrameHeader.Size); | ||
BinaryPrimitives.WriteUInt32BigEndian(_outgoingBuffer.AvailableSpan, ConnectionWindowSize - DefaultInitialWindowSize); | ||
BinaryPrimitives.WriteUInt32BigEndian(_outgoingBuffer.AvailableSpan, windowUpdateAmount); | ||
_outgoingBuffer.Commit(4); | ||
|
||
await _stream.WriteAsync(_outgoingBuffer.ActiveMemory).ConfigureAwait(false); | ||
|
@@ -574,6 +590,7 @@ private void ProcessDataFrame(FrameHeader frameHeader) | |
|
||
if (frameData.Length > 0) | ||
{ | ||
_rttEstimator?.UpdateEstimation(); | ||
ExtendWindow(frameData.Length); | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment isn't correct.
The initial connection window size is defined in the RFC here: https://datatracker.ietf.org/doc/html/rfc7540#section-5.2.1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which part of the commend do you mean? My point is that the initial connection window sizes is a always 65535 according to another section (6.9.2), and can't be configured by SETTINGS frames, therefore we need a different constant.
The initial stream window size, we may want to make it configurable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree we need two constants, I just think the comment is confusing...
"higher values may violate the protocol" -- higher values are completely in violation of the protocol.
I would suggest just calling this InitialConnectionWindowSize and remove the comment entirely, or just comment with a link to https://datatracker.ietf.org/doc/html/rfc7540#section-5.2.1.