Skip to content

Fix HTTP/2 race condition with concurrent window update #38065

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
stephentoub merged 1 commit into from
Jun 18, 2020
+16 −8
Merged

Fix HTTP/2 race condition with concurrent window update #38065

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
9 changes: 7 additions & 2 deletions src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Connection.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1210,7 +1210,7 @@ private async ValueTask<Http2Stream> SendHeadersAsync(HttpRequestMessage request
// Construct and initialize the new Http2Stream instance. It's stream ID must be set below
// before the instance is used and stored into the dictionary. However, we construct it here
// so as to avoid the allocation and initialization expense while holding multiple locks.
var http2Stream = new Http2Stream(request, this, _initialWindowSize);
var http2Stream = new Http2Stream(request, this);

// Start the write. This serializes access to write to the connection, and ensures that HEADERS
// and CONTINUATION frames stay together, as they must do. We use the lock as well to ensure new
Expand All @@ -1233,8 +1233,13 @@ await PerformWriteAsync(totalSize, (thisRef: this, http2Stream, current, remaini
s.thisRef.ThrowShutdownException();
}

// Now that we're holding the lock, configure the stream. The lock must be held while
// assigning the stream ID to ensure only one stream gets an ID, and it must be held
// across setting the initial window size (available credit) and storing the stream into
// collection such that window size updates are able to atomically affect all known streams.
s.http2Stream.Initialize(s.thisRef._nextStream, _initialWindowSize);

// Client-initiated streams are always odd-numbered, so increase by 2.
s.http2Stream.StreamId = s.thisRef._nextStream;
s.thisRef._nextStream += 2;

// We're about to flush the HEADERS frame, so add the stream to the dictionary now.
Expand Down
15 changes: 9 additions & 6 deletions src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Stream.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ private sealed class Http2Stream : IValueTaskSource, IHttpHeadersHandler, IHttpT
// See comment on ConnectionWindowThreshold.
private const int StreamWindowThreshold = StreamWindowSize / 8;

public Http2Stream(HttpRequestMessage request, Http2Connection connection, int initialWindowSize)
public Http2Stream(HttpRequestMessage request, Http2Connection connection)
{
_request = request;
_connection = connection;
Expand All @@ -102,8 +102,6 @@ public Http2Stream(HttpRequestMessage request, Http2Connection connection, int i
_responseBuffer = new ArrayBuffer(InitialStreamBufferSize, usePool: true);

_pendingWindowUpdate = 0;
_availableCredit = initialWindowSize;

_headerBudgetRemaining = connection._pool.Settings._maxResponseHeadersLength * 1024;

if (_request.Content == null)
Expand Down Expand Up @@ -135,13 +133,18 @@ public Http2Stream(HttpRequestMessage request, Http2Connection connection, int i
RequestMessage = _request,
Content = new HttpConnectionResponseContent()
};

if (NetEventSource.IsEnabled) Trace($"{request}, {nameof(initialWindowSize)}={initialWindowSize}");
}

private object SyncObject => this; // this isn't handed out to code that may lock on it

public int StreamId { get; set; }
public void Initialize(int streamId, int initialWindowSize)
{
StreamId = streamId;
_availableCredit = initialWindowSize;
if (NetEventSource.IsEnabled) Trace($"{_request}, {nameof(initialWindowSize)}={initialWindowSize}");
}

public int StreamId { get; private set; }

public HttpResponseMessage GetAndClearResponse()
{
Expand Down