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

If a window update is received from the server between the time when we were creating an Http2Stream (initializing its available credit with the current initial window size) and when we stored the stream into the  streams dictionary, we might miss out on updating the stream's window size, which could in turn lead to stalls.

diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Connection.cs b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Connection.cs
index 846ed88..1e93b2f 100644 (file)
--- a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Connection.cs
+++ b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Connection.cs
@@ -1210,7 +1210,7 @@ namespace System.Net.Http
                 // 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
@@ -1233,8 +1233,13 @@ namespace System.Net.Http
                                 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.

diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Stream.cs b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Stream.cs
index 1372fa5..6fc6b3e 100644 (file)
--- a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Stream.cs
+++ b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Stream.cs
@@ -89,7 +89,7 @@ namespace System.Net.Http
             // 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;
@@ -102,8 +102,6 @@ namespace System.Net.Http
                 _responseBuffer = new ArrayBuffer(InitialStreamBufferSize, usePool: true);
 
                 _pendingWindowUpdate = 0;
-                _availableCredit = initialWindowSize;
-
                 _headerBudgetRemaining = connection._pool.Settings._maxResponseHeadersLength * 1024;
 
                 if (_request.Content == null)
@@ -135,13 +133,18 @@ namespace System.Net.Http
                     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()
             {